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

Move most of TestUtils into TestExt #723

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

penelopeysm
Copy link
Member

@penelopeysm penelopeysm commented Nov 27, 2024

This PR moves the implementations of src/test_utils/context.jl, src/test_utils/sampler.jl, and src/test_utils/varinfo.jl into a new DynamicPPLTestExt extension.

Benefits:

  • This functionality is only used for testing and thus shouldn't really be loaded when the user doesn't want to test. In this sense it is the 'correct' thing to do
  • The benefit of that is, as with any other extension, it reduces load times for users who don't need the functionality

Negatives:

  • Due to the way extensions work in Julia there is no way to namespace functions in an extension. The workaround for this is to define a module plus an empty function inside src/test_utils.jl and then extend this inside TestExt with the real implementation.
  • I don't think this works for types, unfortunately, so I had to leave the definition of one struct inside src/test_utils.jl.

Possible ways forward

  1. Accept this ugly situation and merge this PR.
  2. Do nothing, i.e. close this PR and keep everything inside src.
  3. Move everything to a different package, which can then be declared as a test dep.

My hot take is that I'd prefer (3). Well .... kind of, at least. If we have assorted stuff that builds on DPPL, e.g. benchmarking, I still kind of feel that it would be better to have it in a separate package.

Maybe one possibility is to have the different packages in the same repo so that CI + development doesn't fall out of sync? I recognise this would require a fair bit of reorganisation.

Closes #550

@mhauru
Copy link
Member

mhauru commented Nov 27, 2024

  1. sounds quite confusing to me, although you have documented it. If we go with it, I would add extra documentation explaining why this is needed. But I wonder if the loss of code readability is worth the load time gain. If I was implementing this I would probably go for 2. I fear that 3. would make edits hard and annoying, because of splitting of closely related code between repos. For instance, making a small edit to the test suite that then depends on you making an edit to a separate package sounds like an annoying dev experience.

Base automatically changed from py/refactor-test-utils to master November 27, 2024 15:26
@penelopeysm penelopeysm force-pushed the py/testext branch 2 times, most recently from 298ec36 to 124996e Compare November 27, 2024 17:24
@penelopeysm
Copy link
Member Author

CI has a bunch of UndefVarErrors probably because of moving things around. I won't bother fixing them unless we decide to go ahead with this 😅

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.

Move TestUtils to an extension
2 participants