Add --debug option to run V2 tests interactively. #8827
Conversation
| output_pytest_requirements_pex = 'pytest-with-requirements.pex' | ||
|
|
||
| resolved_requirements_pex = await Get[Pex]( | ||
| CreatePex( |
There was a problem hiding this comment.
There is a fair amount of duplication between this and the python_test_runner rule - we would be able to leverage create_pex_from_target_closure to replace much of this (the only mod it would need is the ability to pass the additional_requirements seen on l51).
There was a problem hiding this comment.
create_pex_from_target_closure is now in master, FWIW.
There was a problem hiding this comment.
The need to duplicate the test runner was one of the motivations for the design from https://docs.google.com/document/d/1Hn73YlhTPROlULTMa_3A-Fdv7hAPiHFII8rvXri5l7E/edit?disco=AAAAD2DjLRk : basically, having a "foreground=True" flag. cc @gshuflin
Before continuing with this implementation, it would be really good to confirm that adding that flag would not be a net simplification over needing to duplicate rules in non-composable ways like this.
There was a problem hiding this comment.
@stuhood @codealchemy n.b. some of these comments were brought up in the context of #8855
The rules necessary to implement --debug should be composeable with the exiting run_python_test rule that yields a TestResult up to here:
This line of code returns an ExecuteProcessRequest and then runs it. The --debug version should take this same ExecuteProcessRequest and extract the fields from it that are relevant to running an InteractiveProcessRequest.
create_execute_request in hermetic_pex.py returns an ExecuteProcessRequest that has the fields:
argv=argv,
input_files=input_files,
description=description,
env=hermetic_env
which are implemented on InteractiveProcessRequest (at least with the above PR which implements input_files), as well as a timeout (which is not currently implemented on InteractiveProcessRequest, but would be easy to do).
It was suggested that InteractiveProcessRequest should perhaps change to just wrap the ExecuteProcessRequest type, rather than duplicate some of its fields. If we go that route, than that makes this --debug rule ever-so-slightly easier to write because then the --debug flag can control whether or not the rule graph asks for the EPR directly or asks for an InteractiveProcessRequest that wraps the EPR.
But either way, this test-debug rule can be implemented by abstraction all of the run_python_test except for the last few lines into a new rule, and then composing that rule with additional rules that get us the new behavior we want, i.e. the ability to get either an ExecuteProcessRequest or InteractiveProcessRequest based on --debug.
There was a problem hiding this comment.
Before continuing with this implementation, it would be really good to confirm that adding that flag would not be a net simplification over needing to duplicate rules in non-composable ways like this.
I don't have a sense of how much work this would be to change. If it's significant, I think I'd prefer to get this in ~as-is (in terms of the overall design, even with the duplication), so we can start working with it, and refactor it to use a proposed --foreground flag in a follow-up, once we better understand the use case and its consequences.
As far as I can tell tests are the only place where this problem arises, so it may end up being the case that the duplication is actually OK, or even better. It may make sense to start off with it and see.
There was a problem hiding this comment.
As far as I can tell tests are the only place where this problem arises, so it may end up being the case that the duplication is actually OK, or even better. It may make sense to start off with it and see.
+1. Don't generalize until we have enough use cases to have a solid understanding of what the generalization needs to support.
There is a fair amount of duplication between this and the python_test_runner rule - we would be able to leverage create_pex_from_target_closure to replace much of this
I'm still concerned about the impact of caching on this. Currently, we produce a requirements PEX with no sources in it. That Pex has a high cache hit rate. Instead, create_pex_from_target_closure would include the source files in the generated Pex so the cache would no longer be useful between different targets as they will always have different source files.
--
Instead, consider adding this debugger implementation directly in python_test_runner.py and deduplicating there, similar to what we do for Black and Isort: https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/python/lint/black/rules.py and https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/python/lint/isort/rules.py.
Until after merged_input_files = await Get(Digest, ...), everything seems duplicated from a quick scan. Introduce a new PytestSetup dataclass that contains the requirements Pex, merged input files, and CLI args, and then have the rules test and debug request a PytestSetup and feed it into an EPR and IPR, respectively.
Happy to pair on this with you.
There was a problem hiding this comment.
I started prototyping the interactive=True flag, and so far it's pretty straightforward. Broken patch here: master...twitter:stuhood/process-execute-foreground
If this PR really needs to land to support a usecase you folks have asap, then I won't block. But please make it easy to deprecate the duplicated code later, as I expect that the foreground patch could be polished in a few more hours of work.
There was a problem hiding this comment.
@stuhood isn't this what InteractiveProcessRunner was meant to do, with the additional constraint that we side-step having to think about how EPR does caching by requiring that InteractiveProcessRunner only execute from console_rules?
There was a problem hiding this comment.
Yes. Except, again, InteractiveProcessRunner requires changes like this PR, which duplicate a lot of code. There would not be any code duplication in the case of foreground=True, because it could be used arbitrarily deep in the rule graph.
Add a MockOptions in the engine util for mocking out rule option values as well as a smoke test running the test rule with debug enabled.
codealchemy
force-pushed
the
python-debug-test-option
branch
from
1535360
to
db3a364
Compare
| output_pytest_requirements_pex = 'pytest-with-requirements.pex' | ||
|
|
||
| resolved_requirements_pex = await Get[Pex]( | ||
| CreatePex( |
There was a problem hiding this comment.
As far as I can tell tests are the only place where this problem arises, so it may end up being the case that the duplication is actually OK, or even better. It may make sense to start off with it and see.
+1. Don't generalize until we have enough use cases to have a solid understanding of what the generalization needs to support.
There is a fair amount of duplication between this and the python_test_runner rule - we would be able to leverage create_pex_from_target_closure to replace much of this
I'm still concerned about the impact of caching on this. Currently, we produce a requirements PEX with no sources in it. That Pex has a high cache hit rate. Instead, create_pex_from_target_closure would include the source files in the generated Pex so the cache would no longer be useful between different targets as they will always have different source files.
--
Instead, consider adding this debugger implementation directly in python_test_runner.py and deduplicating there, similar to what we do for Black and Isort: https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/python/lint/black/rules.py and https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/python/lint/isort/rules.py.
Until after merged_input_files = await Get(Digest, ...), everything seems duplicated from a quick scan. Introduce a new PytestSetup dataclass that contains the requirements Pex, merged input files, and CLI args, and then have the rules test and debug request a PytestSetup and feed it into an EPR and IPR, respectively.
Happy to pair on this with you.
- Update help text. - Move MockOptions out of test utils for now. - Drop fingerprint for options (not needed in V2 world)
### Problem #8811 extracted pex creation to its own rule. There remains a fair amount of duplication in how the pex is created in the `python_test_runner`. ### Solution Allow passing `additional_requirements` in CreatePexFromTargetClosure, which enables the `python_test_runner` to leverage that rule for pex creation. 📝 `include_sources` was added as well to permit creating a pex _without_ the source files from targets - this helps improve cache hits when running tests since we only want to include 3rd party deps. ### Result Less duplicate code and a path forward for #8827 (creating a pex for debugging tests).
This should make it more straightforward to see (and resolve) duplication between the two
Instead of using workspace / tmpdir
fast_* is no longer used across pants, and using `test` alone caused issues when running pytest - this feels like a good middle-ground
| fast_test, | ||
| rule_args=[console, (addr,)], | ||
| run_tests, | ||
| rule_args=[console, options, (addr,)], | ||
| mock_gets=[ | ||
| MockGet( |
There was a problem hiding this comment.
Can you conditionally set mock_gets to either the debug or non-debug Gets based on the value of debug so that we validate in the tests that we don't call into the wrong ones?
There was a problem hiding this comment.
This definitely feels like it'd be helpful here, though AFAICT we can't (currently) condition mock_gets in tests as run_rule uses task_rule.input_gets, which itself is a flat list put together in the _RuleVisitor of all Gets in a rule body (and doesn't account for conditionals).
|
Sorry that I didn't get around to reviewing this in time. Opened #8923 for the followup. |
Problem:
Solution:
--debugoption to thetestconsole rule, which runs a single test target (will error otherwise) interactively.Result: