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

TEST: Improve BatchSimulate testing #955

Open
4 tasks
asoplata opened this issue Nov 26, 2024 · 4 comments
Open
4 tasks

TEST: Improve BatchSimulate testing #955

asoplata opened this issue Nov 26, 2024 · 4 comments
Labels
parallelization Parallel processing testing
Milestone

Comments

@asoplata
Copy link
Collaborator

Currently, testing of BatchSimulate is very incomplete.

  • Importantly, as @ntolley pointed out here ENH: Refactor BatchSimulate Example and Improve Documentation #857 (comment) , we need a way to test that the loky backend not only creates multiple workers, but actually uses the multiple parallel workers, in parallel. I think we may have to actually use loky directly in our tests for this (see ENH: Refactor BatchSimulate Example and Improve Documentation #857 (comment) ).

  • The threading backend only has a single test, the multiprocessing and dask backends are not tested at all.

  • Much of the testing is parameter TypeError testing, which is not testing BatchSimulate's functionality so much as it is testing _validate_type and _check_option. I hate to say it, but I think many of these tests can be removed. Relatedly, I will make an issue for introducing typing and type-checking for the codebase as a whole. I'm fine with leaving these tests in until we get more formal type-checking implemented, however.

  • Relatedly, we should consider dropping the 'multiprocessing' backend as an option, since it has quirks in Joblib and, judging from joblib's documentation, is effectively replaced by loky (see https://joblib.readthedocs.io/en/stable/parallel.html#old-multiprocessing-backend ).

@samadpls
Copy link
Contributor

Hi @asoplata , I agree with everything you've mentioned regarding the testing and using loky. I didn't add the Dask backend for testing, as this repo doesn't currently include it as a dependency, but if needed, it can be installed separately

@asoplata
Copy link
Collaborator Author

It's arguably hacky, but we could also try using psutil to track processes with specific names in our testing, ensuring that the appropriate number are created, etc.

@asoplata
Copy link
Collaborator Author

Hi @asoplata , I agree with everything you've mentioned regarding the testing and using loky. I didn't add the Dask backend for testing, as this repo doesn't currently include it as a dependency, but if needed, it can be installed separately

Thanks @samadpls , yes most of these BatchSimulation test refactors are TODO tasks that existed before your PR. We definitely appreciate your contributions!

@samadpls
Copy link
Contributor

Thank you for the clarification! I will ensure it is useful and maintainable for future development. If you need any changes, please let me know. I'm happy to improve the structure and functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parallelization Parallel processing testing
Projects
None yet
Development

No branches or pull requests

2 participants