-
Notifications
You must be signed in to change notification settings - Fork 156
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
feat: Persistant shell manifests #1080
Conversation
fixes: prefix-dev#1038 Using `Project::discover` to detect if Pixi's manifest path has been set by an environment variable in another `pixi shell` instance. `PIXI_IN_SHELL` is also set to be explicit that this should be an activated environment. A warning is emitted before a command when the project is loaded if the discovered path is different from the one given by the environment variable, along with a warning after most commands in case their outputs are longer and scroll the initial warning off the terminal. This is an alternate take on prefix-dev#1055
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.
I think this makes sense!
We would also need to do this for the environment name. If it is not set we should read it from the env (if PIXI_IN_SHELL is 1).
src/project/mod.rs
Outdated
if std::env::var("PIXI_IN_SHELL").is_ok() { | ||
if let Ok(env_manifest_path) = std::env::var("PIXI_PROJECT_MANIFEST") { | ||
if env_manifest_path.as_str() != project_toml.to_str().unwrap() { | ||
tracing::warn!( | ||
"Using manifest {} from `PIXI_PROJECT_MANIFEST` rather than local {}", | ||
env_manifest_path, | ||
project_toml.to_string_lossy() | ||
); | ||
return Self::load(Path::new(env_manifest_path.as_str())); | ||
} | ||
} | ||
} |
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.
I would change this to only display a warning if the discovered manifest is not equal to the manifest from the environment.
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.
That's what this should do. Are you finding an instance when it's warning more than it should?
Warning when local doesn't match the manifest from environment variable
pixi on manifest-in-shell is 📦 v0.17.1 via 🦀 v1.76.0 took 5s
❯
pixi shell --manifest-path /Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/pixi.toml
. "/var/folders/n6/hl07fcc55ys7mbybt01gb8r80000gn/T/pixi_env_LkI.sh"
pixi on manifest-in-shell is 📦 v0.17.1 via 🦀 v1.76.0
❯ . "/var/folders/n6/hl07fcc55ys7mbybt01gb8r80000gn/T/pixi_env_LkI.sh"
(sea-eagle)
pixi on manifest-in-shell is 📦 v0.17.1 via 🦀 v1.76.0 via 🅒 sea-eagle
❯ pixi info
WARN pixi::project: Using manifest /Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/pixi.toml from `PIXI_PROJECT_MANIFEST` rather than local /Users/akerney/Geek/pixi/pixi.toml
Pixi version: 0.17.1
Platform: osx-arm64
Virtual packages: __unix=0=0
: __osx=14.4.1=0
: __archspec=1=arm64
Cache dir: /Users/akerney/Library/Caches/rattler/cache
Auth storage: /Users/akerney/.rattler/credentials.json
Project
------------
Version: 0.1.0
Manifest file: /Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/pixi.toml
Config locations:
Last updated: 28-03-2024 10:18:19
Environments
------------
Environment: default
Features: default
Channels: conda-forge
Dependency count: 28
Dependencies: python, cftime, dagster-postgres, dagster-k8s, dagster, dask, fsspec, ioos_qc, pandas, pydantic, pystac, python-frontmatter, yaml, requests, sentry-sdk, shapely, xarray, netcdf4, pint, ipykernel, matplotlib, erddapy, uvicorn, xpublish-edr, fastapi, xpublish, xpublish-opendap, uxarray
Target platforms: osx-arm64
Tasks: install-kernel
WARN pixi::project: Used manifest /Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/pixi.toml from `PIXI_PROJECT_MANIFEST` rather than local /Users/akerney/Geek/pixi/pixi.toml
(sea-eagle)
Quiet when they are the same.
pixi on manifest-in-shell [+] is 📦 v0.17.1 via 🦀 v1.76.0 took 5s
❯
pixi shell
. "/var/folders/n6/hl07fcc55ys7mbybt01gb8r80000gn/T/pixi_env_2hk.sh"
pixi on manifest-in-shell is 📦 v0.17.1 via 🦀 v1.76.0
❯ . "/var/folders/n6/hl07fcc55ys7mbybt01gb8r80000gn/T/pixi_env_2hk.sh"
(pixi)
pixi on manifest-in-shell is 📦 v0.17.1 via 🦀 v1.76.0 via 🅒 pixi
❯ pixi info
Pixi version: 0.17.1
Platform: osx-arm64
Virtual packages: __unix=0=0
: __osx=14.4.1=0
: __archspec=1=arm64
Cache dir: /Users/akerney/Library/Caches/rattler/cache
Auth storage: /Users/akerney/.rattler/credentials.json
Project
------------
Manifest file: /Users/akerney/Geek/pixi/pixi.toml
Config locations:
Last updated: 28-03-2024 09:26:37
Environments
------------
Environment: default
Features: default
Channels: conda-forge
Dependency count: 7
Dependencies: pre-commit, rust, openssl, pkg-config, git, cffconvert, tbump
Target platforms: linux-64, win-64, osx-64, osx-arm64
Tasks: install, build, lint, test, test-all, bump
Environment: docs
Features: docs, default
Channels: conda-forge
Dependency count: 13
Dependencies: mkdocs, mkdocs-material, mdx_truly_sane_lists, pillow, cairosvg, mike, pre-commit, rust, openssl, pkg-config, git, cffconvert, tbump
Target platforms: osx-arm64, win-64, osx-64, linux-64
Tasks: install, docs, deploy-dev, deploy-latest, test-all, build, lint, bump, test, build-docs
Environment: schema
Features: schema, default
Channels: conda-forge
Dependency count: 12
Dependencies: python, pytest, jsonschema, pydantic, pyyaml, pre-commit, rust, openssl, pkg-config, git, cffconvert, tbump
Target platforms: osx-64, win-64, linux-64, osx-arm64
Tasks: bump, test-all, build, generate-schema, install, test, lint, test-schema
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.
I don't have time to do a complete review but I found one small thing.
I already like this better, probably best to close the other PR.
Ok, I've taken a look at how to get the environment name from the environment variables. I'm thinking I can add a method to Lines 102 to 104 in 36ac560
/// Tries to read the environment name from an argument, then it will try
/// to read from an environment variable, otherwise it will fall back to default
pub fn from_arg_or_env_var(arg_name: Option<String>) -> Self {
if let Some(arg_name) = arg_name {
return EnvironmentName::Named(arg_name);
} else if std::env::var("PIXI_IN_SHELL").is_ok() {
if let Ok(env_var_name) = std::env::var("PIXI_ENVIRONMENT_NAME") {
return EnvironmentName::Named(env_var_name);
}
}
EnvironmentName::Default
} Which would allow those lines in let environment_name = EnvironmentName::from_arg_or_env_var(args.environment); I'm a little confused about what's different with the Lines 64 to 73 in 36ac560
|
fixes: prefix-dev#893 Currently doesn't work for `pixi run <task>`.
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.
It works now 👍
Thanks @abkfenris !! |
fixes: #1038
Using
Project::discover
to detect if Pixi's manifest path has been set by an environment variable in anotherpixi shell
instance.PIXI_IN_SHELL
is also set to be explicit that this should be an activated environment.A warning is emitted before a command when the project is loaded if the discovered path is different from the one given by the environment variable, along with a warning after most commands in case their outputs are longer and scroll the initial warning off the terminal.
This is an alternate take on #1055