-
Notifications
You must be signed in to change notification settings - Fork 9
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
Improve performance of common subexpression elimination and parallel testing #380
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first part of the PR (common subexpression elimination) is looking very good! I just fixed the typing issue you encountered add added some further notes. I think this part is ready to be be merged.
The second part (parallel tests) also seems to be working just fine. However, I am a little concerned about the limitations [1] concerning a consistent order of test parameters. Random errors thrown by pytest-xdist
(or worse: maybe even missed tests) should be avoided.
Can we guarantee a consistent order? I already verified, that we only use lists in @pytest.mark.parametrize
. It also seems like all functions called to generate such lists produce their output in a well-defined order. However conftest.py
uses e.g. glob
, which uses an arbitrary order. Applying a sort function seems like a reasonable Idea.
-
Please ensure that all tests from
conftest.py
will be in a well-defined order. Please include comments in your work about the necessity of well-defined orders because of parallel testing. -
Do you come to the same conclusion about the "normal" test parameter lists all having a well-defined order?
Thank you for your contribution so far!
[1] https://pytest-xdist.readthedocs.io/en/stable/known-limitations.html
I didn't know about that limitation of pytest-xdist. That is a little annoying. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I will wait for the current run of the extended pipeline to finish and then merge if it all went well.
Currently common subexpression elimination uses lists to store which instructions use a specific expression. This PR replaces this usage with pythons
Counter
class to improve asymptotic runtime.Additionally this pr adds parallelization to tests using pytest-xdist.