search

conda / conda-lock

Lightweight lockfile for conda environments
https://conda.github.io/conda-lock/
Other
458 stars 102 forks source link

Persist packages from original lockfile _only_ for platforms not requested for lock #485

Closed itsarobin closed 10 months ago

itsarobin commented 10 months ago

Description

Addresses #196: for requested platforms replaces lock content without erroneously persisting packages.

TODO:

Co-authored-by: Arie Knoester arikstr@gmail.com Reviewed-by: Matt Fisher mfisher87@gmail.com

netlify[bot] commented 10 months ago

Deploy Preview for conda-lock ready!

Name Link
Latest commit 4fbf8aaf766dafe06d7ff70cbec8042d53a6649e
Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/64ff4a6ed5f5070008cf12e1
Deploy Preview https://deploy-preview-485--conda-lock.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mfisher87 commented 10 months ago

@maresb What do you think of this approach? We have to add some unit tests, but want to check that the approach is sound and take feedback in to account before writing unit tests for what might be a bad idea :smile_cat:

mfisher87 commented 10 months ago

Unit tests for this change are in. We're thinking one more unit test to validate a single-platform update works as expected (that's not testing new functionality, it's testing a feature that pre-dates this change hasn't regressed).

Then we're thinking we will work on variable naming!

mfisher87 commented 10 months ago

While doing this renaming work, I felt the need to refactor. I actually got sucked a little bit in to doing a refactor and then pulled myself back and made this change as simple as I could.

This change made it clear that there's a point midway in make_lock_files where we switch from operating on old lock content to operating on new lock content (I left a comment to that effect), and that's a good indicator we could extract all the behavior that calculates the new lock content into a new function. There are likely even more functions to extract here, the complexity of this function is pretty high. We could get rid of that noqa marker :) My domain knowledge is pretty low, though, so I may need to ask many questions to do such a refactor if you agree it's warranted.

TODO: Rebase again, mark ready for review

mfisher87 commented 10 months ago

@maresb I feel this is ready for a full review :) Thanks for your time thinking about this change!

maresb commented 10 months ago

Thanks so much for this!!!

I'm currently on the road and a bit slammed, so I'm not sure how long it'll take me to give the :+1: but at first glance everything looks good.

I don't like having both requirements-dev.txt and environments/dev-environment.yaml which are not kept in sync. I think we could delete requirements-dev.txt if we simply refactor the mkdocs GitHub workflow to use setup-micromamba. Is this something you'd be able to take care of at the end of this PR? (I'm just thinking that this might be a reasonable add-on since you're already updating the corresponding docs.)

Left: requirements-dev.txt, Right: environments/dev-environment.yaml image

mfisher87 commented 10 months ago

I could probably knock that out! I've used setup-micromamba a few times now. I'll see how the day plays out :)

mfisher87 commented 10 months ago

The test workflow and netlify builds use the requirements-dev.txt, so this ended up being a bit broader than expected. Is the netlify build configured in the repo somewhere, or in a GUI? Maybe removing requirements-dev.txt should be a different PR?

maresb commented 10 months ago

Ah, I'm so sorry, I didn't mean to send you down that rabbit hole!

Let's

  1. Undo the deletion of requirements-dev.txt
  2. Keep mention of requirements-dev.txt out of the docs, since I want to delete it
  3. Get this merged ASAP

I don't have access to Netlify myself, so it's a black box. We'll need help from @mariusvniekerk on this. I tried setting up Netlify through my GitHub, but this is what I end up with:

image

mfisher87 commented 10 months ago

Ah, I'm so sorry, I didn't mean to send you down that rabbit hole!

No sweat at all, I didn't fall in too deep :P I think I've got all the requests done now @maresb!

mfisher87 commented 10 months ago

@maresb I've used nwtgck/actions-netlify@v2 before to do Netlify deploys from GHA (both preview deploys and production deploys). With Actions, dealing with PRs from forks becomes complicated due to GitHub's security restrictions. Using pull_request event type with a fork PR denies access to repo secrets (needed to push to Netlify), and using pull_request_target event type denies access to the changes in the PR, so we can't preview.

One way I've seen to work around this is to have Actions build previews in response to comments (issue_comment event type) by people with a particular role, e.g. owner or maintainer, and containing specific text like /deploy-preview. Usually those projects will also have a bot that responds to new PRs to tell people how to request a preview, e.g.

:robot: Ping @conda-lock-team to request a docs preview. An owner or maintainer must post the comment /deploy-preview to trigger.

mfisher87 commented 10 months ago 
requests.exceptions.HTTPError: 504 Server Error: Gateway Timeout for url: https://api.anaconda.org/download/conda-forge/micromamba/1.5.1/osx-64/micromamba-1.5.1-0.tar.bz2

I'm seeing this error 504 on PR #504 as well (just a coincidence :P )... I'm able to download this file just fine on my local machine.

maresb commented 10 months ago

I'm hoping that the error we're seeing is transient. I'm rerunning the failed tests.

I'm a bit confused by the last commit. Do we really want to offer requirements-dev.txt as an option for contributors to set up their dev environments? I see it more as a CI-only dependency.

Apart from that, this is really wonderful, and really boosts the code quality here! As soon as we clarify the README and the failing test I'm ready to merge.

mfisher87 commented 10 months ago

I'm hoping that the error we're seeing is transient. I'm rerunning the failed tests.

Looks like we're in luck :partying_face:

I'm a bit confused by the last commit

My fault, forgot I need to be pushing to @itsarobin's fork instead of mine :)

Thanks for supporting this change! It's always been a pleasure to interact on this repo :heart:

maresb commented 10 months ago

Thank you @mfisher87! Likewise, it's a pleasure to get such well-curated PRs that I can review in just a few minutes! All this refactoring and cleanup is greatly appreciated.