-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add support for Hatch environments #22779
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.
Good work until now, please ping me @ when it's ready for review. Also, please create an issue corresponding to this PR.
@karrtikr There we go, it’s ready to review! /edit: I also extended the coverage to deal with more than one environment, so I think it’s now pretty complete. |
Should that skip label be added like the failed check says? I just added a setting, so the lockfile should be unaffected |
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.
Thanks for working through this, I'm happy with it overall.
We've yet to do research from our end to make sure it's comprehensive, and that it works as expected. If you could also work on the test plan item to verify it, that would be great. for eg. #21298.
In case this does not support all the hatch scenarios, we might have to revert this PR. So I want to make sure all possible scenarios work before declaring "official" support for it.
src/test/pythonEnvironments/base/locators/lowLevel/hatchLocator.unit.test.ts
Show resolved
Hide resolved
So you mean I should create a checklist of things that should work?
Hmm, I’m not 100% sure I follow, what’s a “scenario”? With Hatch, you
My code therefore calls into Hatch to get all defined environments and then filters them by ones that already exist. Or is all you mean that there’s no corner cases where some existing Hatch environment isn’t found? |
Yep. Here's lifecycle of any new environment type that is added, so say for Hatch:
Based on clarification from @ofek earlier, it seems like we should be able to handle all these scenarios, but we should have a test plan item to confirm this. |
Except for resolving. As said, there is no marker.
They’re regular virtual environments, and they currently have pip installed into them so everything else would, with two caveats regarding installing stuff:
Can we therefore replace the “install pytest?” popup with a non-interactive one that asks the user to add pytest to the configuration? So does the following test plan make sense?
And the following won’t be part of it:
|
Hatch relies exclusively on virtualenv so generally whatever it does will be the behavior. Specifically, pip I think will always be included and that will not change while setuptools I believe no longer is included in 3.12+ environments. |
I added the windows test. could you comment on the rest of the test plan @karrtikr ? |
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.
Hi, thanks for adding the tests and the test plan, I hope to get to it soon latest by end of this week.
Let me know if any other help is required @flying-sheep , we'll be happy to assist with the test plan prototype. @ofek Before we declare official support for Hatch, we want to ensure that when we do switch to the plugin model (where Hatch will need to have it's own extension), someone from the official tool dev community is willing to add support for these settings in the plugin. Could you comment on if someone would be willing to do that? More details in linked comments about the plugin model: #22779 (comment). |
I don't see any talk of that model in your link but yes I would be willing to maintain support. |
Could you link me some example code? I’m happy to extend tests to cover Hatch, but I don‘t have the time to come up with completely new ones for functionality that doesn‘t live in Hatch: Hatch just creates regular venvs, nothing special. They will work just like any other venv. Testing that they work is therefore a formality, so I expected adding tests for things like this to be one, too. |
@ofek Thanks. Regarding the model, basically each tool extension will register an environment provider with us, which will be supposed to implement all the functionality mentioned in this PR. For eg. hatch tool will have a Hatch extension, telling us how to discover, execute etc using such environments. @flying-sheep I'm also working with Pixi regarding that, I can get back to you after an example test plan is ready: #22968 (comment). |
That would be great. Maybe I’ll find the time to dive into this, but I didn’t expect to tread new ground here, I hope you understand. |
@flying-sheep No worries, all the test plan needs to contain is the following: DiscoveryActivation and Execution:
Just elaborating instructions for (?) items should suffice, let me know if you need any other help. |
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.
PR looks good to me as-is so I'm merging this. Please make sure to add the test plan item and we can consider this to be completed, thanks.
I see, so “test plan” doesn’t mean “what I need to implement as unit tests” but “instructions to manually test this”. OK! Is this good? I verified that it works using the newest build. One thing I noticed is that it’s simply reported as “Venv”, not Hatch, probably because Hatch doesn’t have any identifying marker as noticed in the comments talking about resolvers.
|
Looks good, a few adjustments:
Test plan items are tested by VS Code who have little idea about Python, so can you be more specific about instructions for each OS you want this to be tested on? See https://github.com/microsoft/vscode/wiki/Writing-Test-Plan-Items#example-1-platforms for format for such a test plan item.
Right, ideally we should try to report it as Hatch, I think we could have an identifier like Poetry here:
We use regex to identify global poetry envs: vscode-python/src/client/pythonEnvironments/common/environmentManagers/poetry.ts Lines 24 to 42 in 01e5cbd
I think same can be done for Hatch based on the path pattern:
|
I've added a workaround for the identifier issue: #23083, so if an identifier is not registered, we can still correctly discover the type based on where the environment came from. Please try again, now the type should be detected as Hatch. |
…rom locator (#23083) For #22810 ...so in case of Hatch, as an [explicit Hatch identifier is not available](#22779 (comment)), we infer an environment is Hatch if we get it from the Hatch locator: #22779.
Works great, thank you!
No, as it‘s, it’s configurable, dependent on environment variables and so on. That’s what I meant when I talked about “flawed heuristics”: Either we ask Hatch directly or we use a hack. And I don’t think we can ask Hatch for that directory, as there seems to be no command for it.
Hatch has good install instructions, I added them to the plan.
@ofek the GUI installers should make Hatch available in the PATH globally and not just for terminals, right? I’ve seen a lot of hacky instalelrs that muck around with shell specific .somethingrc files instead of using the right system API, but I assume that whatever installer you use does things right. |
Sounds good to me, can you create a corresponding test plan issue for the same? See format and above example items.
Something to elaborate: Perhaps we can ask to print the |
Done! I can’t add labels though. #23088 |
@ofek @flying-sheep Just bringing to your attention that no Hatch environments are being discovered on Windows #23121, due to a bug on hatch. Opened an issue: pypa/hatch#1350. |
Fixes microsoft/vscode-python#22810 TODO - [x] check if it actually works already or if more things need to be registered - [x] add config val - [x] add tests
…rom locator (microsoft/vscode-python#23083) For microsoft/vscode-python#22810 ...so in case of Hatch, as an [explicit Hatch identifier is not available](microsoft/vscode-python#22779 (comment)), we infer an environment is Hatch if we get it from the Hatch locator: microsoft/vscode-python#22779.
Fixes microsoft/vscode-python#22810 TODO - [x] check if it actually works already or if more things need to be registered - [x] add config val - [x] add tests
…rom locator (microsoft/vscode-python#23083) For microsoft/vscode-python#22810 ...so in case of Hatch, as an [explicit Hatch identifier is not available](microsoft/vscode-python#22779 (comment)), we infer an environment is Hatch if we get it from the Hatch locator: microsoft/vscode-python#22779.
Fixes #22810
TODO