Fix missing venv path when running Pyright

[krishnan-chandra]

This fix should address #19368 - I've tested it out locally on my own repo and haven't been able to reproduce the issue so far.

Relevant Slack threads:
https://pantsbuild.slack.com/archives/C046T6T9U/p1687366638498259
https://pantsbuild.slack.com/archives/C046T6T9U/p1688507180323199?thread_ts=1688480012.892699&cid=C046T6T9U

This is my first PR to Pants, so please let me know if there are any steps I forgot!

[WorkerPants]

Welcome to the Pantsbuild Community. This looks like your first PR here.

We are so grateful for each an every contribution, big or small.
So thanks for spending your time and energy making this project better.

If you haven't gotten a reply in a week, feel free to gently comment "bump" to ping the project admins.

If you haven't already, feel free to come say hi on Slack.

If you have questions, or just want to surface this PR, check out the #development channel.
(If you want to check it out without logging in, check out our Linen mirror)

Thanks again for this PR, and we'll be on the lookout for your next one 😄!

[benjyw]

Thanks for tackling this @krishnan-chandra!

I do think John's suggestion over in #19368 (comment) is better than mine though - less finicky and brittle.

That solution involves running a no-op process in the venv. You do something like:

await Get(
        ProcessResult,
        VenvPexProcess(
            requirements_venv_pex,
            description="Force venv to materialize",
            argv=["-c", "''"],
        )
    )

[benjyw]

Thanks @krishnan-chandra. Can you confirm that the problem reproduces without this latest patch and does not reproduce with it?

[krishnan-chandra]

I can indeed! Here are two screenshots:

Before Patch

After Patch

[krishnan-chandra]

For testing, I was thinking about how best to create a test for this case. I am able to reproduce the original issue (and also clearly show that the fix works) by deleting the cache between test runs:

rm -rf ~/.cache/pants/named_caches/pex_root

This seems quite intense for a test though, so I'm wondering if there is a way to delete the path of the requirements venv between test runs. I couldn't figure out how to do that using the test harnesses in https://github.com/pantsbuild/pants/blob/main/src/python/pants/backend/python/typecheck/pyright/rules_integration_test.py, but I'm sure I'm missing something on the internals there.

[benjyw]

Tests are tricky for this because they involve "regular" Pants running a sandboxed Pants (or parts of one). That inner Pants is the one whose named cache you'd want to blow away. It doesn't live in ~/.cache/pants. In the pyright tests, the relevant testing object is the rule_runner (see here for example). In that set_options() call you can set the option for the named caches location to some temp dir you create, and then nuke that.

[krishnan-chandra]

Thanks! I'll take a look at the examples and try to add those tests. In the meantime, are there any other changes that you would like me to make / is there anything else you need before approving the PR?

[benjyw]

No, I think this looks good! And thanks for confirming that it has the desired effect. Just that test (and verifying that the test fails without the patch) and we're good!

[krishnan-chandra]

I added a test - it passes, but I can't get it to fail if I cherry-pick the test onto a branch without the fix. Is there anything obvious that I'm doing wrong there / does the named caches dir need to be passed somewhere else?

[krishnan-chandra]

I've pushed up what I think should be a better test - but again, I can't get this one to fail consistently even though I can definitely get Pants to fail when running against my own monorepo.

[benjyw]

I've pushed up what I think should be a better test - but again, I can't get this one to fail consistently even though I can definitely get Pants to fail when running against my own monorepo.

Hmm, this would be absolutely key. If the test doesn't fail without the change then it is not really testing what we think it is...

[krishnan-chandra]

Definitely agreed there - I'm not super familiar with Pants internals so it's been a bit challenging to chase down the source of the problem. I'll keep pushing on it. In the meantime, if someone more familiar with the Pyright integration has suggestions, I am all ears :)

[benjyw]

I'll take a look when I get a moment.

[benjyw]

OK, so one thing complicating this test is that it's too simple: Specifically, it doesn't actually require any venv to be present in order to pass. And so it passes regardless of the presence of absence of a venv (the --venvPath not existing is just a pyright warning, not an error).

So the next step here is to make the test sufficiently complex to actually require some 3rdparty dep to be present in order to pass.

[benjyw]

Hi @krishnan-chandra , have you had a chance to look at this test again?

[krishnan-chandra]

I did revisit it, and I tried to add some third-party imports to force the venv to materialize. One thing I couldn't quite work out is where to look for the output. Here is my test output:

Command:

./pants --keep-sandboxes=always test src/python/pants/backend/python/typecheck/pyright/rules_integration_test.py -- -k test_passing_cache_clear

Output:

19:21:17.08 [INFO] Preserving local process execution dir /private/var/folders/0w/zhpm9fs12zzcmt3m5799xb9c0000gn/T/pants-sandbox-AivibN for Searching for `bash` on PATH=/usr/bin:/bin:/usr/local/bin
19:21:17.26 [INFO] Preserving local process execution dir /private/var/folders/0w/zhpm9fs12zzcmt3m5799xb9c0000gn/T/pants-sandbox-G5A9c6 for Test binary /bin/bash.
19:21:17.31 [INFO] Preserving local process execution dir /private/var/folders/0w/zhpm9fs12zzcmt3m5799xb9c0000gn/T/pants-sandbox-BnaswX for Extracting plugin locations
19:21:18.92 [INFO] Initializing scheduler...
19:21:32.03 [INFO] Scheduler initialized.
19:21:33.97 [INFO] Preserving local process execution dir /private/var/folders/0w/zhpm9fs12zzcmt3m5799xb9c0000gn/T/pants-sandbox-kwvimH for Find interpreter for constraints: CPython==3.10.*
19:21:35.77 [INFO] Preserving local process execution dir /private/var/folders/0w/zhpm9fs12zzcmt3m5799xb9c0000gn/T/pants-sandbox-7NoP1T for Searching for `bash` on PATH=/usr/bin:/bin:/usr/local/bin
19:21:35.88 [INFO] Preserving local process execution dir /private/var/folders/0w/zhpm9fs12zzcmt3m5799xb9c0000gn/T/pants-sandbox-Rr5xKf for Test binary /bin/bash.
19:21:35.94 [INFO] Preserving local process execution dir /private/var/folders/0w/zhpm9fs12zzcmt3m5799xb9c0000gn/T/pants-sandbox-L322Gu for Run Pytest for src/python/pants/backend/python/typecheck/pyright/rules_integration_test.py:tests
19:21:49.25 [ERROR] Completed: Run Pytest - src/python/pants/backend/python/typecheck/pyright/rules_integration_test.py:tests - failed (exit code 1).
============================= test session starts ==============================
collecting ... collected 14 items / 13 deselected / 1 selected

src/python/pants/backend/python/typecheck/pyright/rules_integration_test.py::test_passing_cache_clear FAILED [100%]

=================================== FAILURES ===================================
___________________________ test_passing_cache_clear ___________________________

rule_runner = RuleRunner(build_root=/private/tmp/_BUILD_ROOToahgp6be)

    def test_passing_cache_clear(rule_runner: PythonRuleRunner) -> None:
        # Ensure that the requirements venv must be created, by adding in a third-party
        # requirement to the test code.
        rule_runner.write_files({
            "BUILD": "python_requirement(name='more-itertools', requirements=['more-itertools==8.4.0'])",
            f"{PACKAGE}/f.py": dedent(
                """\
                from more_itertools import flatten

                assert flatten(42) == [4, 2]
                """
            ),
            f"{PACKAGE}/BUILD": "python_sources()",
        })
        print("Rule runner is running from directory: ", rule_runner.build_root)
        with temporary_dir() as named_caches:
            print("Temporary cache dir located at: ", named_caches)
            tgt = rule_runner.get_target(Address(PACKAGE, relative_file_path="f.py"))
            result = run_pyright(rule_runner, [tgt], extra_args=[f"--named-caches-dir={named_caches}"])
            assert len(result) == 1
>           assert result[0].exit_code == 0
E           assert 1 == 0
E             +1
E             -0

src/python/pants/backend/python/typecheck/pyright/rules_integration_test.py:163: AssertionError
----------------------------- Captured stdout call -----------------------------
Rule runner is running from directory:  /private/tmp/_BUILD_ROOToahgp6be
Temporary cache dir located at:  /tmp/tmpf9u5rvy1
- generated xml file: /private/var/folders/0w/zhpm9fs12zzcmt3m5799xb9c0000gn/T/pants-sandbox-L322Gu/src.python.pants.backend.python.typecheck.pyright.rules_integration_test.py.tests.xml -

This makes sense as I largely got the implementation from test_thirdparty_dependency, which is supposed to fail. What's weird though is that I can't find either of these directories even though I'm running with --keep-sandboxes=always:

/private/tmp/_BUILD_ROOToahgp6be
/tmp/tmpf9u5rvy1

And the following command does not yield anything:

find /private/var/folders/0w/zhpm9fs12zzcmt3m5799xb9c0000gn -name f.py 2> /dev/null

[benjyw]

temporary_dir() is a context manager that deletes the tmpdir when the context ends (i.e., outside the with block). You can temporarily use temporary_dir(cleanup=False) to prevent that, but you do want the test to clean up after itself. This means that you need to do any checking of that dir inside the with block. This tmpdir is separate from any process sandbox.

The /private/tmp/_BUILD_ROOToahgp6be is also a tmpdir, created somewhere in run_pyright's callstack, so, again, is gone by the time you get to inspect it.

Does that make sense? Is that helpful?

[benjyw]

Thanks for your diligence here @krishnan-chandra !