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 locator for pixi environments #22968

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

baszalmstra
Copy link

@baszalmstra baszalmstra commented Feb 26, 2024

Closes #22978

This adds a locator implementation that properly detects Pixi environments. Pixi environments are essentially conda environments but placed in a specific directory inside the project/workspace. This PR properly detects these and does not do much else. This would unblock a lot of pixi users.

I would prefer to use a custom pixi plugin but since the contribution endpoints are not available yet I think this is the next best thing.

Before I put more effort into tests I just want to verify that this approach is valid. Let me know what you think! :)

@baszalmstra
Copy link
Author

@microsoft-github-policy-service agree company="Prefix.dev GmbH"

@karthiknadig karthiknadig added the feature-request Request for new features or functionality label Feb 26, 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 the PR, please create an issue corresponding to the same, we hope to get to this shortly.

@baszalmstra
Copy link
Author

Thanks for the PR, please create an issue corresponding to the same, we hope to get to this shortly.

Ofc, see #22978

package.json Outdated Show resolved Hide resolved
@baszalmstra
Copy link
Author

I took the time to also add tests. These are based on the ones from hatch and poetry.

@karrtikr karrtikr changed the title feat: add locator for pixi environments Add locator for pixi environments Mar 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 the work, I'm happy with it overall. If you could also work on the test plan item to verify it, that would be great. for eg. #21298, #21102.

See #22779 (comment) for similar ask from another tool, feel free to let me know if there's any questions,

@karrtikr karrtikr added the skip package*.json package.json and package-lock.json don't both need updating label Mar 11, 2024
@baszalmstra
Copy link
Author

Looking at #22779 (comment) this PR currently only adds discovery, (I would like to work on the rest after this).

Would a test-plan like this suffice?

Requirements:

  • Have a working pixi installation (>0.15.2), installation instructions can be found here: https://pixi.sh/ .

Verification:

  • Open a new empty workspace folder.
  • Open a terminal in the workspace and run the following commands to initialize a pixi project with a python interpreter:
    pixi init . 
    pixi add python
    
  • Add an script.py file to the root of the workspace and open it (the contents doesnt matter).
  • Verify that a default interpreter has been selected that points to python interpreter in the .pixi\envs directory of the workspace. Its type should be Pixi.

image

@pavelzw
Copy link

pavelzw commented Mar 11, 2024

@baszalmstra maybe also add multi envs to the test plan?

@karrtikr
Copy link

karrtikr commented Mar 15, 2024

@pavelzw That sounds fine for discovery, I think this PR might also cover additional scenarios, as pixi environments are essential conda under the hood:

Activation and Execution:

#22779 (comment)

Installation:

  • Select a pixi environment
  • Open a new terminal
  • Install something using pip
  • Make sure it is installed

If this doesn't work, we might need to add a pixiInstaller.ts similar to

export class CondaInstaller extends ModuleInstaller {

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.

Merge conflicts ⚔️ Here's an example test plan item for the Discovery & Activation part: #23088. Please @ me once this is done, unsubscribing for now.

@martinhulin
Copy link

Looking at #22779 (comment) this PR currently only adds discovery, (I would like to work on the rest after this).

Would a test-plan like this suffice?

Requirements:

  • Have a working pixi installation (>0.15.2), installation instructions can be found here: https://pixi.sh/ .

Verification:

  • Open a new empty workspace folder.
  • Open a terminal in the workspace and run the following commands to initialize a pixi project with a python interpreter:
    pixi init . 
    pixi add python
    
  • Add an script.py file to the root of the workspace and open it (the contents doesnt matter).
  • Verify that a default interpreter has been selected that points to python interpreter in the .pixi\envs directory of the workspace. Its type should be Pixi.

image

Will this also work for R?

@karrtikr karrtikr assigned karthiknadig and unassigned karrtikr Mar 27, 2024
@baszalmstra
Copy link
Author

I have added implementations of activation, a module installer, and execution. Will add a test plan one of these days!

@pavelzw
Copy link

pavelzw commented Apr 16, 2024

@karrtikr @karthiknadig whats the current state of this PR? I would love to get this available in vscode 😍

@karthiknadig
Copy link
Member

@baszalmstra Is this ready for review?

@baszalmstra
Copy link
Author

Sorry for leaving this for so long. I am on a holiday but will be back next week.

Im quite happy with the functionality provided by this PR. I think the biggest thing missing is the test plan. Also some unit tests might still be required, I could definitely use some help with those.

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.

Pixi environment locator
6 participants