Use search-strategy: all for all site packages in pyre config #25
Conversation
I'm trying to find a way to support, in Pyre1, dual development modes where pytorch might be installed via site-packages but alternatively might be installed from source in the same project. If we try to use a regular `search_path` it won't work; on my machine I don't have `../pytorch`, and Pyre will crash complaining about a missing search path elment. Luckily, though, we added an optional_search_path with exactly the behavior we need a while back: Pyre1 will ignore it if the path doesn't exist, and include it if it does. Note that
.pyre_configuration
Outdated
| @@ -10,12 +10,8 @@ | |||
| "source": "examples" | |||
| } | |||
| ], | |||
| "optional_search_path": ["../pytorch"], | |||
There was a problem hiding this comment.
Regarding:
Note that there's a tradeoff here: the fact that ../pytorch gets precedence this means if there is a ../pytorch directory but we are not intending to use it in the current development environment, we're still going to pick it up. That could lead to confusion if it's mismatched against the current conda env, but I don't really see a way around it.
Piggy-backing on Jason's idea in a chat, perhaps we can create two .pyre_configuration files (one has "optional_search_path" and one doesn't) , and decide which one to use based on torch.__file__ in the current conda env?
There was a problem hiding this comment.
Does the all change alone suffice?
Any idea why python setup.py develop breaks the old config? The files should still be symlinked from site-packages.
There was a problem hiding this comment.
Any idea why python setup.py develop breaks the old config? The files should still be symlinked from site-packages.
I'm not entirely sure what's going on here, it's been a while since I looked deeply at Pyre's import logic and it was never terribly well optimized for external use cases. My best guess is that it isn't following the links properly
Does the all change alone suffice?
I'm actually not 100% sure why the "all" change is needed, but when I dropped the explicit search paths it couldn't find "triton" - my guess is that this is because triton is missing a py.typed marker file, which PEP561 says is required
perhaps we can create two .pyre_configuration files
You definitely could have two configurations, but you'd have to swap them out manually which might be annoying. A wrapper script could certainly do it
.pyre_configuration
Outdated
| @@ -10,12 +10,8 @@ | |||
| "source": "examples" | |||
| } | |||
| ], | |||
| "optional_search_path": ["../pytorch"], | |||
There was a problem hiding this comment.
Can you also add "../pytorch-hg", this might be only for me but i have multiple pytorch checkouts, one git one mercurial
This PR attempts to make Pyre capable of checking using either installed
torchfrom site packages, or if it is available atorchcoming from a repository at../pytorch, relative to the currenthelionrepository.The first diff improves our options for making a local pytorch repo work, because it eliminates the rigid assumption that we'll find
torchin the site-packages directory (otherwise, Pyre will crash complaining about an invalid search path entry).It's also generally simpler, and eliminates other potential consequences of trying to enumerate the site pachage entries; this is the go-to mode I would generally use for Pyre.
Tested by setting up a virtualenv, making sure
pyre checkworks on trunk, and seeing that I get a clean result after this config change.The second diff uses an "optional_search_path" to specify that
../pytorchshould be used as a search path, but only if it exists. I verified that:../pytorch, this will run as before../pytorch, it winds up as a search root, and precedes site-packagesNote that there's a tradeoff here: the fact that
../pytorchgets precedence this means if there is a../pytorchdirectory but we are not intending to use it in the current development environment, we're still going to pick it up. That could lead to confusion if it's mismatched against the current conda env, but I don't really see a way around it.