Skip to content
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

Merged
merged 16 commits into from
Mar 15, 2024
Merged

Conversation

flying-sheep
Copy link

@flying-sheep flying-sheep commented Jan 23, 2024

Fixes #22810

TODO

  • check if it actually works already or if more things need to be registered
  • add config val
  • add tests

@karthiknadig karthiknadig added the feature-request Request for new features or functionality label Jan 23, 2024
Copy link

@karrtikr karrtikr left a 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.

@flying-sheep
Copy link
Author

flying-sheep commented Feb 2, 2024

@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.

@flying-sheep flying-sheep marked this pull request as ready for review February 2, 2024 13:04
@flying-sheep
Copy link
Author

Should that skip label be added like the failed check says? I just added a setting, so the lockfile should be unaffected

@karrtikr karrtikr added the skip package*.json package.json and package-lock.json don't both need updating label Feb 7, 2024
Copy link

@karrtikr karrtikr left a 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.

@flying-sheep
Copy link
Author

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.

So you mean I should create a checklist of things that should work?

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.

Hmm, I’m not 100% sure I follow, what’s a “scenario”?

With Hatch, you

  1. define environments statically
  2. have them be created when running something via hatch run

My code therefore calls into Hatch to get all defined environments and then filters them by ones that already exist.
That’s it. Is that enough? Does our implementation need to also be able to create environments or something?

Or is all you mean that there’s no corner cases where some existing Hatch environment isn’t found?

@karrtikr
Copy link

karrtikr commented Feb 8, 2024

So you mean I should create a checklist of things that should work?

Yep. Here's lifecycle of any new environment type that is added, so say for Hatch:

  • Discovery: Locate all hatch environments in the system, you've done a good job with this.
  • Resolving: This feature enables us to get all information from a discovered environment. For eg. the identifier mentioned earlier is part of it.
  • Execution: Can we execute a Python file with dependencies successfully using this Hatch environment?
  • Activation: Are we able to activate this environment successfully once a new terminal is opened up?
  • Installation: Can we install packages into this environment? For eg. when testing, we prompt users to install pytest, does clicking "yes" on that notification work?

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.

@flying-sheep
Copy link
Author

flying-sheep commented Feb 9, 2024

Based on clarification from @ofek earlier, it seems like we should be able to handle all these scenarios

Except for resolving. As said, there is no marker.

but we should have a test plan item to confirm this.

They’re regular virtual environments, and they currently have pip installed into them so everything else would, with two caveats regarding installing stuff:

  1. It’s not a great idea to install things into Hatch environments manually, as the next hatch prune will remove them again.
  2. Maybe Hatch will, like Rye, at some point stop pre-installing setuptools and pip into its environments, at which point I don’t know if things will work with your setup.

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?

  • Discovery: Do we correctly locate all hatch environments for a given project
    • linux
    • windows
  • Execution: Can we execute a Python file with dependencies successfully using this Hatch environment?
  • Activation: Are we able to activate this environment successfully once a new terminal is opened up?
  • Installation: Is there a non-interactive popup shown asking users to add pytest? Deferred, see below

And the following won’t be part of it:

  • Resolving: We could at most create a flawed heuristic, as there is no marker.
  • Installation: Should be no problem to do, but doesn’t make sense for Hatch’s model.

@karrtikr karrtikr self-requested a review February 9, 2024 12:13
@ofek
Copy link

ofek commented Feb 9, 2024

Maybe Hatch will, like Rye, at some point stop pre-installing setuptools and pip into its environments, at which point I don’t know if things will work with your setup.

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.

@flying-sheep
Copy link
Author

flying-sheep commented Feb 19, 2024

I added the windows test. could you comment on the rest of the test plan @karrtikr ?

Copy link

@karrtikr karrtikr left a 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.

.vscode/launch.json Outdated Show resolved Hide resolved
@karrtikr
Copy link

karrtikr commented Mar 8, 2024

Poetry or other locators were actually not verified via test plan items (assuming that's what is meant by test files), please use #21298 as a reference regarding the format. Here's an example test plan which verifies activation: #21102.

@karrtikr
Copy link

karrtikr commented Mar 12, 2024

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).

@ofek
Copy link

ofek commented Mar 12, 2024

I don't see any talk of that model in your link but yes I would be willing to maintain support.

@flying-sheep
Copy link
Author

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.

@karrtikr
Copy link

karrtikr commented Mar 14, 2024

@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).

@flying-sheep
Copy link
Author

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.

@karrtikr
Copy link

@flying-sheep No worries, all the test plan needs to contain is the following:

Discovery

#22968 (comment)

Activation and Execution:

  • Install some packages in the hatch environment (how?)
  • Select it as an interpreter using Python: Select Interpreter command
  • Create a python file using those packages (maybe include an example file?)
  • Run it using the play icon on top right (maybe include screenshot of the button?)
  • The file is successfully run (maybe print something at the end of the file to indicate it ran successfully?)

Just elaborating instructions for (?) items should suffice, let me know if you need any other help.

Copy link

@karrtikr karrtikr left a 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.

@karrtikr karrtikr changed the title Add Hatch venv locator Add support for Hatch environments Mar 15, 2024
@VSCodeTriageBot VSCodeTriageBot added this to the March 2024 milestone Mar 15, 2024
@karrtikr karrtikr enabled auto-merge (squash) March 15, 2024 20:28
@karrtikr karrtikr merged commit cd3ea27 into microsoft:main Mar 15, 2024
35 checks passed
@flying-sheep flying-sheep deleted the hatch-locator branch March 16, 2024 11:43
@flying-sheep
Copy link
Author

flying-sheep commented Mar 16, 2024

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.


  1. Have a working Hatch installation (https://hatch.pypa.io/latest/install/)
  2. Make sure hatch is runnable in a terminal as described there
  3. In an empty project, add two files:
    1. testproj.py containing
      import session_info
      
      session_info.show()
    2. … and a pyproject.toml specifying dependencies:
      [project]
      name = "testproj"
      version = "0.1.0"
      dependencies = ["session-info"]
      
      [build-system]
      requires = ["hatchling"]
      build-backend = "hatchling.build"
  4. Make Hatch install the project and its dependencies to an environment using hatch run ..., e.g. hatch run python -V (there is no configuration necessary for Hatch to work in a simple case like this)
  5. Select the testproj venv as an interpreter using Python: Select Interpreter command (It might be at the very bottom of the list)
    grafik
  6. Run it using the play icon on top right
    grafik
  7. The file is successfully run and prints the session info:
    grafik
  8. Open a new terminal: If you have the python.terminal.activateEnvironment setting set to true, the environment should be activated.

@karrtikr
Copy link

Looks good, a few adjustments:

Have a working Hatch installation added it to your PATH (e.g. by using pipx)

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.

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.

Right, ideally we should try to report it as Hatch, I think we could have an identifier like Poetry here:

export async function isPoetryEnvironment(interpreterPath: string): Promise<boolean> {

We use regex to identify global poetry envs:

/**
* Global virtual env dir for a project is named as:
*
* <sanitized_project_name>-<project_cwd_hash>-py<major>.<micro>
*
* Implementation details behind <sanitized_project_name> and <project_cwd_hash> are too
* much to rely upon, so for our purposes the best we can do is the following regex.
*/
const globalPoetryEnvDirRegex = /^(.+)-(.+)-py(\d).(\d){1,2}$/;
/**
* Checks if the given interpreter belongs to a global poetry environment.
* @param {string} interpreterPath: Absolute path to the python interpreter.
* @returns {boolean} : Returns true if the interpreter belongs to a venv environment.
*/
async function isGlobalPoetryEnvironment(interpreterPath: string): Promise<boolean> {
const envDir = getEnvironmentDirFromPath(interpreterPath);
return globalPoetryEnvDirRegex.test(path.basename(envDir)) ? isVirtualenvEnvironment(interpreterPath) : false;
}

I think same can be done for Hatch based on the path pattern: "/home/node/.local/share/hatch/env/virtual/<hatchproject>/sZNvn4Gf/<hatchproject>"

".../env/virtual/<hatchproject>/sZNvn4Gf/<hatchproject>" part looks to be following a pattern at least.

@karrtikr
Copy link

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.

karrtikr pushed a commit that referenced this pull request Mar 17, 2024
…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.
@flying-sheep
Copy link
Author

flying-sheep commented Mar 17, 2024

Works great, thank you!

I think same can be done for Hatch based on the path pattern

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.

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?

Hatch has good install instructions, I added them to the plan.
For windows and macOS there’s GUI installers, but for Linux, an up-to-date version of Hatch is only packaged system wide for a few distributions, most notably Arch and Fedora, but no Ubuntu or Debian. So the hard part here is that without a system wide installation and without a hatchPath setting, VS Code won’t find hatch. This applies to some Linux distributions, but not others:

  • Windows, macOS: Install via GUI installer: https://hatch.pypa.io/latest/install/ (easy)
  • Arch, Fedora, …: Install via system package manager (easy)
  • Linux distributions that add ~/.local/bin to the PATH by default (e.g. RPM based ones): install via pipx (easy)
  • Ubuntu, …: Here it’s hard to add hatch to the PAH in a way that VS Code can see it. I think just adding to the ~/.profile doesn’t do the trick

@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.

@karrtikr
Copy link

karrtikr commented Mar 17, 2024

Sounds good to me, can you create a corresponding test plan issue for the same? See format and above example items.

If you have the python.terminal.activateEnvironment setting set to true, the environment should be activated.

Something to elaborate: Perhaps we can ask to print the sys.executable and check whether it's the same as hatch env find.

@flying-sheep
Copy link
Author

Done! I can’t add labels though. #23088

@karrtikr
Copy link

Awesome. Can you update the screenshots here? In the latest one it should be appearing under the "Hatch" group instead of "Venv":

image

@karrtikr
Copy link

@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.

wesm pushed a commit to posit-dev/positron that referenced this pull request Apr 5, 2024
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
wesm pushed a commit to posit-dev/positron that referenced this pull request Apr 5, 2024
…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.
wesm pushed a commit to posit-dev/positron that referenced this pull request Apr 8, 2024
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
wesm pushed a commit to posit-dev/positron that referenced this pull request Apr 8, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality skip package*.json package.json and package-lock.json don't both need updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Hatch environment discovery
6 participants