Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose tests and benchmarks as public sublibrary #481

Merged
merged 8 commits into from
Jan 30, 2024
Merged

Conversation

Shimuuar
Copy link
Contributor

This was discussed in #426 and #459 as a way for other packages to access tests and benchmarks from vector. I tested that it's possible to build PAPI-based benchmarks as separate package, but this change is big enough that it deserves its own PR

There're two tests sublibraries: one built with with -O2 (vector:tests-O2) and another with -O0 (vector:tests-O0). In the same way benchmarks library is called vector:benchmarks-O2. I want to leave name for benchmarks built with -O1 since testing both -O1 and -O2 could be quite valuable.

Most notable change: building vector requires cabal 3.0 since it's earliest version supporting public sublibraries.

There's also question of naming of modules in both sublibraries. I left them as they were but it was assumed that they are not available outside of test-suite/benchmarks which is not the case anymore.

This is minimal version that supports public sublibraries.
This exposes benchmarks internal for use with other benchmark tools. In this
case for benchmarks based on CPU counters
@Shimuuar
Copy link
Contributor Author

There's also problem with cabal check. It complains:

Warning: These warnings may cause trouble when distributing the package:
Warning: 'ghc-options: -O0' is not needed. Use the --disable-optimization
configure flag.
Warning: 'ghc-options: -O0' is not needed. Use the --disable-optimization
configure flag.

and fails CI. It seems there's no good way around this. We do need to build tests with different optimization levels

@lehins
Copy link
Contributor

lehins commented Jan 28, 2024

It also complains about bounds:

Warning: These packages miss upper bounds:
- base-orphans
- random
- tasty
- tasty-hunit
- tasty-quickcheck
- template-haskell
- transformers

I think we can just disable cabal check, since those warnings are useless.

@Shimuuar
Copy link
Contributor Author

Cabal check has warnings and warning-but-actually-errors. Former are printed but cabal check returns 0, latter causes non-zero exit code. AFIAR hackage runs cabal check on upload and will reject packages failing it, warnings are accepted.

Maybe there's way around this by structuring tests different;y. But I think it would be better to keep tests private in this PR

Copy link
Contributor

@lehins lehins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe cabal check is right? Is it actually necessary to specify optimization flag for libraries?
I thought it is up the end executable to control that. If this is true, then all we need is two sublibraries that would specify all of the reusable functionality:

  • testlib
  • and benchlib

and then me can have the separate test suites and bemchmarks with varying optimization levels.

I also agree that module names should be improved before we actually make a release.

My suggestion would be adding these prefixes:

  • for test library Test.Vector...
  • for benchmarks Bench.Vector...

Tests.Vector.Boxed
Tests.Vector.Storable
Tests.Vector.Primitive
Tests.Vector.Unboxed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to not expose all of the functionality from a sub-library.

The idea behind the library is that downstream users would be able to use it somehow. For example Arbitrary instances are quite useful. Potentially some properties implemented in terms of generic vector, in case someone wants to test their custom instances of vector, like instance of Unboxed vector.

That being said, we can make these improvements later, so it doesn't have to be done in this PR

@Shimuuar
Copy link
Contributor Author

Yes I think sublibrary for tests turned out to be trickier than I thought. It would be better to keep tests as they are. And extract common functionality piece by piece later.

With benchmarks situation with optimization flags is more tricky. They export monomorphic function and optimization level is decided when function is built. Application cannot change it.

It turns out turning tests into library is not easy thing to do. So just
common stanza is used to factor out common functionality for tests

Also drop HUinit dependency, we're using tasty-hunit
They are used by PAPI based benchmarks as well
This is:
 a) Much more readable
 b) harder to forget to force something
@Shimuuar
Copy link
Contributor Author

So I left tests mostly alone and only factored shared parts into common stanza. Also:

  • Dropped HUnit dependency

For benchmarks sublibrary I moved all modules into Bench.Vector hierarcy and moved RandomSeed & VectorSize options to library as well. They will be used by PAPI benchmark as well

Copy link
Contributor

@lehins lehins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!

@Shimuuar Shimuuar merged commit 85297ff into master Jan 30, 2024
22 checks passed
@Shimuuar Shimuuar deleted the cabal-sublibs branch January 30, 2024 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants