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

feat: Persistant shell manifests (using CLI env) #1055

Closed
wants to merge 14 commits into from

Conversation

abkfenris
Copy link
Contributor

fixes: #1038

Initial attempt to support persistent shell manifest context.

When running pixi shell the PIXI_PROJECT_MANIFEST environment variable is set, and other commands that take --manifest_path will read from there.

If the environment variable is set, and the path hasn't been overridden, Pixi will emit a warning.
WARN pixi::project: Using manifest `/Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/pixi.toml` from `PIXI_PROJECT_MANIFEST` environment variable.

Trying out if `--manifest-path` can be set by environment variable. I munged the environment activation and the arguments for info and list to try this out.
fixes: prefix-dev#1038

Initial attempt to support persistent shell manifest context.

When running `pixi shell` the `PIXI_PROJECT_MANIFEST` environment variable is set, and other commands that take `--manifest_path` will read from there.

If the environment variable is set, and the path hasn't been overridden, Pixi will emit a warning. `WARN pixi::project: Using manifest `/Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/pixi.toml` from `PIXI_PROJECT_MANIFEST` environment variable`.
@abkfenris
Copy link
Contributor Author

Currently some commands like add and remove cause PIXI_PROJECT_MANIFEST to be reset to the current directory, after they make the changes to the correct manifest.

pixi on  manifest-env-var [+] is 📦 v0.17.1 via 🦀 v1.76.0 via 🅒 sea-eagle
❯ pixi add uxarray
 WARN pixi::project: Using manifest `/Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/pixi.toml` from `PIXI_PROJECT_MANIFEST` environment variable
✔ Added uxarray
(sea-eagle)
pixi on  manifest-env-var [+] is 📦 v0.17.1 via 🦀 v1.76.0 via 🅒 pixi took 4s
❯ pixi info
 WARN pixi::project: Using manifest `/Users/akerney/Geek/pixi/pixi.toml` from `PIXI_PROJECT_MANIFEST` environment variable

@abkfenris
Copy link
Contributor Author

Add, remove, and install call environment::get_up_to_date_prefix

pixi/src/cli/add.rs

Lines 368 to 374 in 214d913

get_up_to_date_prefix(
&project.default_environment(),
lock_file_usage,
no_install,
sparse_repo_data,
)
.await?;

pixi/src/environment.rs

Lines 175 to 209 in 214d913

pub async fn get_up_to_date_prefix(
environment: &Environment<'_>,
lock_file_usage: LockFileUsage,
mut no_install: bool,
existing_repo_data: IndexMap<(Channel, Platform), SparseRepoData>,
) -> miette::Result<Prefix> {
let current_platform = Platform::current();
let project = environment.project();
// Do not install if the platform is not supported
if !no_install && !environment.platforms().contains(&current_platform) {
tracing::warn!("Not installing dependency on current platform: ({current_platform}) as it is not part of this project's supported platforms.");
no_install = true;
}
// Make sure the project is in a sane state
sanity_check_project(project)?;
// Ensure that the lock-file is up-to-date
let mut lock_file = project
.up_to_date_lock_file(UpdateLockFileOptions {
existing_repo_data,
lock_file_usage,
no_install,
..UpdateLockFileOptions::default()
})
.await?;
// Get the locked environment from the lock-file.
if no_install {
Ok(Prefix::new(environment.dir()))
} else {
lock_file.prefix(environment).await
}
}

Which calls lock_file.prefix()

impl<'p> LockFileDerivedData<'p> {
/// Returns the up-to-date prefix for the given environment.
pub async fn prefix(&mut self, environment: &Environment<'p>) -> miette::Result<Prefix> {
if let Some(prefix) = self.updated_pypi_prefixes.get(environment) {
return Ok(prefix.clone());
}
// Get the prefix with the conda packages installed.
let platform = Platform::current();
let (prefix, python_status) = self.conda_prefix(environment).await?;
let repodata_records = self
.repodata_records(environment, platform)
.unwrap_or_default();
let pypi_records = self.pypi_records(environment, platform).unwrap_or_default();
let uv_context = match &self.uv_context {
None => {
let context = UvResolutionContext::from_project(self.project)?;
self.uv_context = Some(context.clone());
context
}
Some(context) => context.clone(),
};
let env_variables = environment.project().get_env_variables(environment).await?;
// Update the prefix with Pypi records
environment::update_prefix_pypi(
environment.name(),
&prefix,
platform,
&repodata_records,
&pypi_records,
&python_status,
&environment.system_requirements(),
uv_context,
env_variables,
self.project.root(),
)
.await?;
// Store that we updated the environment, so we won't have to do it again.
self.updated_pypi_prefixes
.insert(environment.clone(), prefix.clone());
Ok(prefix)
}

If I stick a debug on line 122 where it gets environment variables, it runs twice, and seems to loose the context the second time:

❯ pixi remove uxarray
 WARN pixi::project: Using manifest `/Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/pixi.toml` from `PIXI_PROJECT_MANIFEST` environment variable
Removed uxarray >=2024.2.0,<2024.3 from [dependencies]
[src/lock_file/update.rs:122] environment.project().get_env_variables(environment).await? = {
    "PIXI_ENVIRONMENT_NAME": "default",
    "PIXI_PROJECT_NAME": "sea-eagle",
    "XML_CATALOG_FILES": "file:///Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/.pixi/envs/default/etc/xml/catalog file:///etc/xml/catalog file:///Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/.pixi/envs/default/etc/xml/catalog file:///etc/xml/catalog",
    "CONDA_DEFAULT_ENV": "sea-eagle",
    "PIXI_PROJECT_VERSION": "0.1.0",
    "PIXI_PROJECT_MANIFEST": "/Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/pixi.toml",
    "PATH": "/Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/.pixi/envs/default/bin:/Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/.pixi/envs/default/bin:/Users/akerney/micromamba/condabin:/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/Users/akerney/.cargo/bin:/Users/akerney/.local/bin:/Users/akerney/.local/bin:/Users/akerney/.pixi/bin",
    "PIXI_ENVIRONMENT_PLATFORMS": "osx-arm64",
    "PIXI_PROMPT": "(sea-eagle) ",
    "PIXI_EXE": "/Users/akerney/.cargo/bin/pixi",
    "PIXI_PROJECT_ROOT": "/Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S",
}
[src/lock_file/update.rs:122] environment.project().get_env_variables(environment).await? = {
    "CONDA_DEFAULT_ENV": "pixi",
    "PIXI_EXE": "/Users/akerney/.cargo/bin/pixi",
    "PIXI_PROJECT_MANIFEST": "/Users/akerney/Geek/pixi/pixi.toml",
    "PIXI_PROJECT_NAME": "pixi",
    "PIXI_PROJECT_VERSION": "NO_VERSION_SPECIFIED",
    "GSETTINGS_SCHEMA_DIR_CONDA_BACKUP": "",
    "PATH": "/Users/akerney/Geek/pixi/.pixi/envs/default/bin:/Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/.pixi/envs/default/bin:/Users/akerney/micromamba/condabin:/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/Users/akerney/.cargo/bin:/Users/akerney/.local/bin:/Users/akerney/.local/bin:/Users/akerney/.pixi/bin",
    "PIXI_PROMPT": "(pixi) ",
    "GSETTINGS_SCHEMA_DIR": "/Users/akerney/Geek/pixi/.pixi/envs/default/share/glib-2.0/schemas",
    "CONDA_PREFIX": "/Users/akerney/Geek/pixi/.pixi/envs/default",
    "PIXI_PROJECT_ROOT": "/Users/akerney/Geek/pixi",
    "PIXI_ENVIRONMENT_NAME": "default",
    "PIXI_ENVIRONMENT_PLATFORMS": "linux-64,osx-64,osx-arm64,win-64",
}

@ruben-arts
Copy link
Contributor

I think its better to put some logic in the discover() instead of the load as we don't interfere with the --manifest-path set by the user.

src/activation.rs Outdated Show resolved Hide resolved
dependabot bot and others added 2 commits March 25, 2024 17:04
Bumps the dependencies group with 1 update: [mozilla-actions/sccache-action](https://github.com/mozilla-actions/sccache-action).


Updates `mozilla-actions/sccache-action` from 0.0.3 to 0.0.4
- [Release notes](https://github.com/mozilla-actions/sccache-action/releases)
- [Commits](Mozilla-Actions/sccache-action@v0.0.3...v0.0.4)

---
updated-dependencies:
- dependency-name: mozilla-actions/sccache-action
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: dependencies
...

Signed-off-by: dependabot[bot] <support@github.com>
Add, remove, and install commands were causing the environment to get reset as `pixi shell-hook` would get re-run.

Removes redundant setting of PIXI_PROJECT_MANIFEST environment variable.
@abkfenris
Copy link
Contributor Author

I think its better to put some logic in the discover() instead of the load as we don't interfere with the --manifest-path set by the user.

The logic in load is to show a warning when it's discovered the manifest via environment variable and it hasn't been overridden. The environment variable and --manifest-path both use load() rather than discover.

> pixi shell --manifest-path /Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/pixi.toml
 . "/var/folders/n6/hl07fcc55ys7mbybt01gb8r80000gn/T/pixi_env_lpQ.sh"

Warn when --manifest-path is set from environment:

(sea-eagle)
pixi on  manifest-env-var 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` environment variable
      Pixi version: 0.17.1
          Platform: osx-arm64
  Virtual packages: __unix=0=0
                  : __osx=14.4=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: 25-03-2024 13:43:46

Environments
------------
       Environment: default
          Features: default
          Channels: conda-forge
  Dependency count: 27
      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
  Target platforms: osx-arm64
             Tasks: install-kernel

Don't warn when --manifest-path is set directly.

(sea-eagle)
pixi on  manifest-env-var is 📦 v0.17.1 via 🦀 v1.76.0 via 🅒 sea-eagle
❯ pixi info --manifest-path ./pixi.toml
      Pixi version: 0.17.1
          Platform: osx-arm64
  Virtual packages: __unix=0=0
                  : __osx=14.4=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: 22-03-2024 13:46:21

Environments
------------
       Environment: default
          Features: default
          Channels: conda-forge
  Dependency count: 11
      Dependencies: pre-commit, rust, openssl, pkg-config, git, pytest, jsonschema, pydantic, pyyaml, cffconvert, tbump
  Target platforms: linux-64, osx-arm64, win-64, osx-64
             Tasks: generate-schema, test-all, lint, test, build, test-schema, bump, install

       Environment: docs
          Features: docs, default
          Channels: conda-forge
  Dependency count: 17
      Dependencies: mkdocs, mkdocs-material, mdx_truly_sane_lists, pillow, cairosvg, mike, pre-commit, rust, openssl, pkg-config, git, pytest, jsonschema, pydantic, pyyaml, cffconvert, tbump
  Target platforms: win-64, osx-arm64, linux-64, osx-64
             Tasks: build-docs, deploy-dev, docs, lint, deploy-latest, test-schema, bump, install, generate-schema, test, test-all, build

I really like that clap shows the environment variables that it's picking up as defaults.

(sea-eagle)
pixi on  manifest-env-var is 📦 v0.17.1 via 🦀 v1.76.0 via 🅒 sea-eagle
❯ pixi info --help
Information about the system, project and environments for the current machine

Usage: pixi info [OPTIONS]

Options:
      --extended                       Show cache and environment size
      --json                           Whether to show the output as JSON or not
      --manifest-path <MANIFEST_PATH>  The path to 'pixi.toml' [env:
                                       PIXI_PROJECT_MANIFEST=/Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/pixi.toml]
  -v, --verbose...                     Increase logging verbosity
  -q, --quiet...                       Decrease logging verbosity
      --color <COLOR>                  Whether the log needs to be colored [env: PIXI_COLOR=] [default: auto] [possible
                                       values: always, never, auto]
  -h, --help                           Print help

The issue with the environment getting reset by add/remove/install was that I hadn't set the shell hook to use the environment variable as an argument, so it fell back to discover and found the nearest manifest to reset the environment variable too.

@abkfenris abkfenris marked this pull request as ready for review March 25, 2024 19:04
@abkfenris
Copy link
Contributor Author

I've updated the docs, but I'm not sure how I'd go about testing this in Rust.

@ruben-arts
Copy link
Contributor

I think your remarks are valid. Clap does have a nice output.

One question I have is, do you think it makes sense to support this feature without it forcing the user to have an activated env. Currently setting the PIXI_PROJECT_PATH would not activate the env but it would always use that pixi project when you run anything. Then the location of anything you pixi run will also be relative to the location of the project.

I think there is something to say for having more then one variable, e.g. a activation timestamp which is hard to fake, so users don't get unexpected behavior.

I see that the warning is already making it more clear but a lot of commands have long and fast output which would make the warning vanish. This could potentially be fixed but making sure it is the last thing pixi outputs instead of the first.

@abkfenris
Copy link
Contributor Author

One question I have is, do you think it makes sense to support this feature without it forcing the user to have an activated env. Currently setting the PIXI_PROJECT_PATH would not activate the env but it would always use that pixi project when you run anything. Then the location of anything you pixi run will also be relative to the location of the project.

I think there is something to say for having more then one variable, e.g. an activation timestamp which is hard to fake, so users don't get unexpected behavior.

I thought about this some when I first started tinkering (and then put additional env variables on the back burner as I am very new to Rust and it would have added more complexity than I was ready to tackle). Personally, I think it does make sense to not require an activated environment, and I think it will be less unexpected than pixi shell's current behavior.

Right now pixi shell activates the conda environment, but doesn't set Pixi's working context to be the same as that environment. By setting PIXI_PROJECT_MANIFEST with pixi shell (and reading it with other commands) both environment activation and Pixi's working context are in sync.

I think that by leaving the environment variable settable by users, Pixi will allow supporting other workflows (#949, #255, #188, #1021 for example) that don't fall into the regular Pixi workflow vision. This gives those users an escape hatch and lets them configure Pixi's working context via direnv, mise, .profile and similar.

So far, I haven't configured pixi shell's --manifest-path to be settable by PIXI_PROJECT_MANIFEST, but now I see that it would be more confusing to run that an not have it activate the environment that Pixi's working context would have otherwise specified for those users. I'll tweak that now.

I see that the warning is already making it more clear but a lot of commands have long and fast output which would make the warning vanish. This could potentially be fixed but making sure it is the last thing pixi outputs instead of the first.

With a warning before, it gives the user a change to exit before a potentially unintended change, so I think a warning should at least stay, but I added one afterwards. I'm not sure it is necessarily needed for all commands, but I added it to all that are using --manifest-path.

pixi on  manifest-env-var [!] is 📦 v0.17.1 via 🦀 v1.76.0 took 2m49s
❯
pixi shell --manifest-path /Users/akerney/GMRI/NERACOOS/NERACOOS_ERDDAP_K8S/pixi.toml
 . "/var/folders/n6/hl07fcc55ys7mbybt01gb8r80000gn/T/pixi_env_Zbo.sh"

pixi on  manifest-env-var is 📦 v0.17.1 via 🦀 v1.76.0
❯  . "/var/folders/n6/hl07fcc55ys7mbybt01gb8r80000gn/T/pixi_env_Zbo.sh"
(sea-eagle)
pixi on  manifest-env-var 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` environment variable. May have been set by `pixi shell`.
      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: 26-03-2024 20:46:30

Environments
------------
       Environment: default
          Features: default
          Channels: conda-forge
  Dependency count: 27
      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
  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` environment variable. May have been set by `pixi shell`.
(sea-eagle)

@pavelzw
Copy link
Contributor

pavelzw commented Mar 28, 2024

Shouldn't pixi also persist the active environment? #893

@abkfenris
Copy link
Contributor Author

Shouldn't pixi also persist the active environment? #893

Good catch, added that in. I so far haven't been using multiple environments, so I hadn't dug into how they worked.

@ruben-arts
Copy link
Contributor

He @abkfenris, sorry to be so slow on this PR but we don't think the current implementation is what we want in pixi.

We would like to keep the CLI as it was before and move all this logic into the discover function.
Here we would not only check the PIXI_MANIFEST_PATH but also a PIXI_IN_SHELL variable (that does not exist yet) to really show pixi that you are in an active environment. This also shows the user that wants to spoof it that this is a requirement.

The warning given to the user should be thrown only when we believe there could be unexpected behavior, so only when the user is in a different pixi project than the activated project we should spawn the warning. This is also important to keep the user sharp on warnings.

This might not be a solution to the issue @pavelzw shared but that should be seen.

@pavelzw
Copy link
Contributor

pavelzw commented Mar 28, 2024

but also a PIXI_IN_SHELL variable

will this be added to pixi shell-hook as well?

@ruben-arts
Copy link
Contributor

will this be added to pixi shell-hook as well?

Yes it should.

abkfenris added a commit to abkfenris/pixi that referenced this pull request Mar 28, 2024
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
@abkfenris abkfenris changed the title feat: Persistant shell manifests feat: Persistant shell manifests (using CLI env) Mar 28, 2024
@abkfenris
Copy link
Contributor Author

Ok, I've made another PR that tries to implement things using discover and a PIXI_IN_SHELL environment variable. #1080

@abkfenris
Copy link
Contributor Author

Closing in favor of #1080

@abkfenris abkfenris closed this Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants