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

Bump to recent versions, and make bootstrap.py update to those when run #719

Merged

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Oct 19, 2021

PR summary

I saw quite a bit of very specific pinnings, all the way to the patch version. I figure that is problematic so I made it instead pin the latest major version, or in case no major version has been released to the first minor version.

I updated:

  • the tljh package dependencies installed with the root python environment
  • the hub environment requirements, installed by tljh
  • the user environment requirements, installed by tljh
  • the ensure_pip/conda_package functions to accept a flag to make packages be upgraded to resolve Running pip install oauthenticator==1.* should include the --upgrade flag - right? #732 introduced by not pinning strictly any more
    • --upgraded added to pip install
    • --update-deps and --update-spec added to conda install
      My intention is that this should update the package and its dependencies, just like I expect pip install --upgrade works.

Related

@consideRatio consideRatio force-pushed the pr/update-hub-user-env-and-tljh-deps branch from 86869a7 to d38f6db Compare October 19, 2021 10:00
@consideRatio consideRatio changed the title pr/update hub user env and tljh deps Update and relax config requirements Oct 19, 2021
@consideRatio consideRatio marked this pull request as draft October 19, 2021 10:06
@consideRatio consideRatio force-pushed the pr/update-hub-user-env-and-tljh-deps branch from d38f6db to e966f64 Compare October 19, 2021 10:09
tljh/installer.py Outdated Show resolved Hide resolved
@consideRatio consideRatio force-pushed the pr/update-hub-user-env-and-tljh-deps branch 3 times, most recently from 4cdd45b to 1ea9115 Compare October 20, 2021 19:33
@consideRatio consideRatio force-pushed the pr/update-hub-user-env-and-tljh-deps branch 2 times, most recently from 072154e to cf24256 Compare October 20, 2021 21:37
@consideRatio consideRatio marked this pull request as ready for review October 20, 2021 21:51
Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Looks great to me. I'll defer to @GeorgianaElena on the chardet question.

For fully frozen environments, it would be best to use pip-tools or conda lockfiles to separate the 'actual' loosely defined dependencies from the fully frozen ones. I think it's good to move away from manually managed exact pins.

@consideRatio consideRatio force-pushed the pr/update-hub-user-env-and-tljh-deps branch from c0671e2 to 15be1c1 Compare October 27, 2021 07:16
@consideRatio consideRatio changed the title Update and relax config requirements Bump to recent versions, and make bootstrap.py update to those when run Oct 27, 2021
@consideRatio consideRatio force-pushed the pr/update-hub-user-env-and-tljh-deps branch from 15be1c1 to 1dab737 Compare October 27, 2021 07:18
@consideRatio consideRatio marked this pull request as ready for review October 27, 2021 07:23
@consideRatio
Copy link
Member Author

@yuvipanda @minrk I've rebased this PR on #721 now - could you re-review it?

Since @minrk this PR approved, I've added the commit 1dab737 that makes us do pip install --upgrade when installing some things that are no longer pinned to a specific version, but instead pinned to something like oauthenticator==1.*.

setup.py Show resolved Hide resolved
Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

I think this is good to go once the tests pass

@consideRatio
Copy link
Member Author

@yuvipanda I did conda install --update-deps --update-specs but the flags were exclusive. I wanted to mirror pip install --upgrade but I wasn't sure how to accomplish it. Do you have ideas on what the equivalent variant would be?

To clarify, my assumption is that if I do pip install --upgrade package==1.* that has some dependencies, but the package and the dependencies will get updated. My assumption about conda install is that only the package will be updated unless some flags are passed, but I'm very uncertain about conda in general.

Conda flags details

[--freeze-installed | --update-deps | -S | --update-all | --update-specs]

  --freeze-installed, --no-update-deps
                        Do not update or change already-installed dependencies.
  --update-deps         Update dependencies.
  -S, --satisfied-skip-solve
                        Exit early and do not run the solver if the requested specs are satisfied. Also skips aggressive updates as configured by 'aggressive_update_packages'. Similar to the
                        default behavior of 'pip install'.
  --update-all, --all   Update all installed packages in the environment.
  --update-specs        Update based on provided specifications.

@consideRatio
Copy link
Member Author

consideRatio commented Oct 27, 2021

I'm thinking that either we want to have --update-specs or no flag at all for conda. Hmmm...

I think --update-deps is the right call, I'm not 100%, but I think so.

EDIT: aaargh well it seems like dependencies are updated by default. I'm not changing conda to update anything explicitly with a flag.

Since we now longer pin versions to the patch version, we should make
an install cause the packages upgrade within the version constraints
rathern than just settle for the current version if it is already
installed.
@consideRatio consideRatio force-pushed the pr/update-hub-user-env-and-tljh-deps branch from 536b055 to 125e12c Compare October 27, 2021 08:42
@consideRatio
Copy link
Member Author

Final summary

@yuvipanda, I made pip install --upgrade be used now that we don't declare exact versions, but instead declare versions like oauthenticator==1.*. I reverted changes to the mamba install command regarding something equivalent to the pip --upgrade flag, as conda/mamba seem to update the deps by default.

tljh/conda.py Outdated
'install',
'--no-cache-dir',
] + packages)
pip_cmd = pip_executable + ['install', '--no-cache-dir']
Copy link
Member

@minrk minrk Oct 27, 2021

Choose a reason for hiding this comment

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

Not for this PR, but we we probably don't want to call pip with --no-cache-dir. This is common as a space optimization for keeping images small, but isn't the right thing for any pip install on a persistent VM.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh note I retained --no-cache-dir, but I can remove it part of this PR, makes sense to me to remove!

tljh/conda.py Outdated
'-r',
requirements_path
])
pip_cmd = pip_executable + ['install', '--no-cache-dir']
Copy link
Member

Choose a reason for hiding this comment

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

Actually, now that I see you've added --no-cache-dir here, let's not do that. And you can remove it above, as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, you are right, I added it here! Woops!

Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Minor comment that we shouldn't use --no-cache-dir with pip, but LGTM!

@consideRatio
Copy link
Member Author

consideRatio commented Oct 28, 2021

With recent approvals (and fix of removing --no-cache-dir), ill go for a merge on this!

Thank you @minrk and @yuvipanda for the review efforts!!

@consideRatio consideRatio merged commit 2d6d970 into jupyterhub:main Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants