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

Switch to Mamba #697

Merged
merged 7 commits into from Oct 19, 2021
Merged

Switch to Mamba #697

merged 7 commits into from Oct 19, 2021

Conversation

manics
Copy link
Member

@manics manics commented Jul 31, 2021

  • Add / update documentation
  • [ ] Add tests Can't remember why I added this item since the tests were already in place and just needed updating for mambaforge

Switches from conda to mamba. The main advantage is this should require less memory (I've reduced the integration test Docker limit to 900 MB), hopefully this is enough to get TLJH running on AWS t2.micro instances with 1GB RAM.

I haven't updated any docs yet, I'd like some feedback first.

@welcome

This comment has been minimized.

@consideRatio
Copy link
Member

I'd love to see us use mambaforge instead of miniconda, big upvote on this PR! I'd be quick to review any additional changes to docs etc.

I'd love to have someone deliberate on if this is a breaking change or not. I'm not sure myself, but I note that we have tests that first install the current version of things via the bootstrap script currently on GitHub in the repo's default branch, and that we then run it again etc.

hopefully this is enough to get TLJH running on AWS t2.micro instances with 1GB RAM.

This relates to #671 and #696.

tljh/installer.py Outdated Show resolved Hide resolved
installer_sha256 = "a012c24e1cc3bcbe74a1e5693e510830e7c2956e85877b08d1e28707a0bd8d75"
mamba_version = '0.15.2'

if conda.check_miniconda_version(USER_ENV_PREFIX, mambaconda_new_version):
Copy link
Member

Choose a reason for hiding this comment

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

Do you understand the idea about this? It would be great to have a comment around here regarding these checks.

Copy link
Member Author

@manics manics Oct 18, 2021

Choose a reason for hiding this comment

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

I did figure it out eventually but I can't remember the details now! It's something along the lines of check the conda version, from which you can infer the miniconda version that was used, and from that you can infer how old the TLJH installation is.

Edit: Or maybe it's the other way round, figure out which version of miniconda is installed, then choose which version of conda we want based on that (maybe because older miniconda versions installed an older version of Python which isn't suportted by newer conda?)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I consider this non-blocking for a merge. I've heard about feature creep, but this is more like a maintenance creep of me to ask about :D

tljh/installer.py Outdated Show resolved Hide resolved
manics and others added 3 commits October 18, 2021 22:09
@manics
Copy link
Member Author

manics commented Oct 18, 2021

I've rebased this PR, and I think I've responded to everything apart from #697 (comment)

@consideRatio
Copy link
Member

@GeorgianaElena @yuvipanda what do you think about this?

@yuvipanda
Copy link
Collaborator

Lets' do this!

@yuvipanda yuvipanda merged commit ebd76f7 into jupyterhub:master Oct 19, 2021
@welcome
Copy link

welcome bot commented Oct 19, 2021

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@yuvipanda
Copy link
Collaborator

I'd love to have someone deliberate on if this is a breaking change or not. I'm not sure myself, but I note that we have tests that first install the current version of things via the bootstrap script currently on GitHub in the repo's default branch, and that we then run it again etc.

I think the upgrade logic we have means this isn't a 'breaking' change as such :)

@manics manics deleted the mamba branch October 19, 2021 08:55
@consideRatio
Copy link
Member

❤️ 🎉 WIEEEEEEEEEEEEEEE!!!

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.

None yet

3 participants