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

--force-reinstall old conda to ensure it's working before we try to install conda packages #920

Merged
merged 5 commits into from Jun 9, 2023

Conversation

minrk
Copy link
Member

@minrk minrk commented Jun 9, 2023

an old conda bug causes RemoveError: requests is a dependency of conda when installing other packages (closes #918).

Upgrading conda itself first avoids this bug (we _may_ want `--force` here, which some folks have seen required when conda itself is messed up, but let's avoid it for now). So we know that the version in 0.2.0 (4.10) is not actually new enough to work. It's unclear what the true minimum version is to fix this, but the important thing is that it will be upgraded if it's still the version in 0.2.0 (4.10), so pick the versions from today which we know works, and will be the version upgraded-to if an 0.2 install has yet to update conda.

conda install --force-reinstall conda is the fix for this, because conda install conda will fail to upgrade if we're in this situation. We could make it conditional on old condas, as I don't think this is likely to come up with more recent versions, but it should be safe in general to run this command.

minrk added 2 commits June 9, 2023 13:58
an old conda bug causes RemoveError: requests is a dependency of conda
when installing other packages

upgrading conda itself avoids this bug.
It's unclear what the true minimum version is to fix this,
but the important thing is that it will be upgraded if it's still the version in 0.2.0 (4.10),
so pick the version from today which we know works
if conda upgrade conda is broken, we are in trouble

this is required to avoid the RemoveError
@minrk
Copy link
Member Author

minrk commented Jun 9, 2023

It appears to be a little tricky to upgrade conda itself when the env is considered broken like this. Working on it.

not deprecated --force
@minrk
Copy link
Member Author

minrk commented Jun 9, 2023

If we don't want to bump versions, it appears the fix is specifically:

conda install --force-reinstall conda

which should ensure that conda itself is in a consistent state (and should not upgrade it). We could do that as a separate, unconditional 'ensure conda is okay' step before installing anything, but I believe using --force-reinstall to ensure that conda/mamba are super consistent ought to be good enough.

We could also try to restrict 'needs force' to a version constraint, but I have no idea what that constraint would be.

@consideRatio
Copy link
Member

We could also try to restrict 'needs force' to a version constraint, but I have no idea what that constraint would be.

I think its great if we avoid introducing that precautionary, as it makes the situations that can arise grow and its harder to ensure function in all situations. For the same reasons, I'm happy about seeing more stringent requirements on the conda/mamba versions.

@minrk
Copy link
Member Author

minrk commented Jun 9, 2023

--force-reinstall behaves a bit weird. It doens't upgrade packages, so unconstrained versions result in no upgrade unless the version constraint is in the install command.

I think maybe it makes sense to unconditionally force-reinstall conda/mamba to ensure it's always consistent, followed by the upgrade steps.

@minrk minrk force-pushed the upgrade-mamba-conda branch 2 times, most recently from ec0bcb5 to c032291 Compare June 9, 2023 12:46
makes it more likely that subsequent conda installs will succeed

- fix indentation on the upgrade steps so they aren't run every iteration
- no longer need to bump required versions
@minrk minrk changed the title bump minimum conda/mamba versions needed for upgrade --force-reinstall conda to ensure it's working before we try to install conda packages Jun 9, 2023
USER_ENV_PREFIX, list(to_reinstall), force_reinstall=True
)

cf_pkgs_to_upgrade = list(set(to_upgrade) & {"conda", "mamba"})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines are unchanged, but they are dedented because they should have always been run after the for loop, not on each iteration. Results should be unchanged, it was just a few unnecessary calls to conda install:

# first 'conda' iteration
conda install conda
# second 'mamba' iteration
conda install conda mamba
# third 'pip' iteration
conda install conda mamba
pip install --upgrade pip

when it should have just been the last two calls.

@consideRatio consideRatio mentioned this pull request Jun 9, 2023
1 task
@minrk
Copy link
Member Author

minrk commented Jun 9, 2023

Struggling to get the right behavior here to match elaborate test expectations. It seems --force-reinstall does upgrade when run with old versions, unlike more recent ones.

It really seems the 'reinstall if needed' is what we want, but I can't seem to figure out 'if needed'.

avoids upgrade on older versions
@consideRatio
Copy link
Member

I searched on reinstall in https://github.com/conda/conda/blob/main/CHANGELOG.md, and found one entry released for 4.4.0 that included at least some kind of change conda/conda#5836.

# avoids issues with RemoveError: 'requests' is a dependency of conda
# only do this for 'old' conda versions known to have a problem
# we don't know how old, but we know 4.10 is affected and 23.1 is not
if not is_fresh_install and V(package_versions.get("conda", "0")) < V("23.1"):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like what I ended up doing here, but I bracketed the force reinstall on a version older than what's going to be in 1.0, with the hope that we actually remove this step when we drop support for upgrading from earlier than 1.0.

I think you need pretty old conda to get here, but that's what we have in 0.2. It really is a special-case to handle:

  • quite old conda
  • inconsistent environment in conda's own dependencies (caused by our own pip install --upgrade commands)

but this is what you'll get if you start with tljh 0.2 and haven't upgraded conda, so we should handle it. I think it is appropriate to treat it specially, as if users have broken their own envs such that conda itself doesn't work, I think it's appropriate for us to not handle that and require users to fix their conda (or delete their user env to start fresh) before upgrading. But since we are creating a broken environment, we need to handle it, at least for the very specific case of upgrading from a previous tljh known to have this issue.

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, go for merge?

THANK YOU FOR WORKING THIS @minrk !!!!!!!!!!

@minrk
Copy link
Member Author

minrk commented Jun 9, 2023

phew everything's passing and test expectations are unchanged.

One thing that might help these things (or might not, honestly, since it creates an extra env to manage) is to make the root conda install a separate environment from the user env:

  • conda root env - /opt/tljh/conda
  • user env created with /opt/tljh/conda/bin/conda create --prefix /opt/tljh/user

Then conda/mamba themselves won't actually interact with user packages. This is where the pip install step caused problems by upgrading requests, a dependency of conda itself. There would be no reason to have pip packages in the root env if we did this.

@minrk
Copy link
Member Author

minrk commented Jun 9, 2023

all set from my side, yes

@minrk minrk changed the title --force-reinstall conda to ensure it's working before we try to install conda packages --force-reinstall old conda to ensure it's working before we try to install conda packages Jun 9, 2023
@consideRatio
Copy link
Member

consideRatio commented Jun 9, 2023

I'd love to see tljh 1.0.0 installations - either fresh installs or upgrades, arrive to an even more solid baseline that we can count on going onwards. But that would be a separate discussion!

One thing that might help these things (or might not, honestly, since it creates an extra env to manage) is to make the root conda install a separate environment from the user env:

Yikes, as a maintainer I just feel that I've caught up with the understanding of:

  • system env (bootstrap.py runs here, setups hub and user env)
  • hub env (tljh installer runs here, setups jupyterhub in the env)
  • user env (initialized with misc things, but the only essential part is jupyterhub singleuser logic)

I think the problems solved should be major to justify an additional environment because of the complexity of even just understanding the purpose of each, and documenting that, etc.

@consideRatio consideRatio merged commit 55eec52 into jupyterhub:main Jun 9, 2023
13 checks passed
@yuvipanda
Copy link
Collaborator

btw, I'm still seeing this on https://github.com/jupyterhub/the-littlest-jupyterhub/actions/runs/6357501669/job/17268703149#step:5:157 from #942 (which is based on latest main)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade from 0.2.0 fails (because a corrupt conda/mamba file?)
3 participants