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

itsarobin · 2023-08-20T23:59:06Z

Description

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

TODO:

Add unit tests

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

netlify · 2023-08-20T23:59:11Z

✅ Deploy Preview for conda-lock ready!

Name	Link
🔨 Latest commit	`4fbf8aa`
🔍 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... Use your smartphone camera to open QR code link.

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

mfisher87 · 2023-08-21T00:02:42Z

@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 😸

conda_lock/conda_lock.py

mfisher87 · 2023-08-21T00:19:18Z

conda_lock/conda_lock.py

+                    for dep in lock_content.package
+                    if dep.platform not in platforms_to_lock
+                ]
+                filtered_lock_content = lock_content.copy(


Better name: lock_content_to_persist?

Ya. I'd even rename the original to original_lock_content. And maybe new_lock_content → fresh_lock_content in order to reserve new_lock_content for the final result.

I'm a big fan of long and descriptive variable names.

Ya. I'd even rename the original to original_lock_content. And maybe new_lock_content → fresh_lock_content in order to reserve new_lock_content for the final result.

I'm a big fan of long and descriptive variable names.

💯 same here :)

- `lock_content` -> `old_lock_content` - `new_lock_content` -> `fresh_lock_content` - `filtered_lock_content` -> `lock_content_to_persist` - create var `new_lock_content` from merge of `filtered_lock_content` and `fresh_lock_content`

mfisher87 · 2023-08-28T14:44:30Z

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!

environments/README.md

mfisher87 · 2023-09-10T16:04:36Z

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

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

…or lock Addresses conda#196: for requested platforms replaces lock content without erroneously persisting packages. Co-authored-by: Arie Knoester <arikstr@gmail.com> Reviewed-by: Matt Fisher <mfisher87@gmail.com>

Co-authored-by: Arie Knoester <arikstr@gmail.com>

mfisher87 · 2023-09-10T16:26:23Z

conda_lock/conda_lock.py

-        assert lock_content is not None
+        # After this point, we're working with `new_lock_content`, never
+        # `original_lock_content` or `fresh_lock_content`.
+        assert new_lock_content is not None


I found this assertion a bit confusing. I was not able to type new_lock_content: Lockfile because original_lock_content could be None when we do this assignment:

if not platforms_to_lock: new_lock_content = original_lock_content

I would like to move this check in to the type system, but that seems better left for a larger refactor effort.

mfisher87 · 2023-09-10T16:28:36Z

conda_lock/conda_lock.py

@@ -385,9 +385,12 @@ def make_lock_files(
            )
        platforms_to_lock = sorted(set(platforms_to_lock))

-        if platforms_to_lock:
+        if not platforms_to_lock:
+            new_lock_content = original_lock_content


Since previously we were mutating lock_content in this conditional block, the "else" condition was lack of mutation. I think making that case explicit and first made this renaming change the most readable.

mfisher87 · 2023-09-10T16:36:36Z

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

maresb · 2023-09-10T17:46:35Z

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 👍 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

mfisher87 · 2023-09-10T19:14:52Z

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

mfisher87 · 2023-09-10T23:58:15Z

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 · 2023-09-11T03:22:10Z

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

Let's

Undo the deletion of requirements-dev.txt
Keep mention of requirements-dev.txt out of the docs, since I want to delete it
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:

mfisher87 · 2023-09-11T14:32:22Z

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 · 2023-09-11T14:44:36Z

@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.

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

mfisher87 · 2023-09-11T15:05:00Z

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 · 2023-09-11T17:03:39Z

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 · 2023-09-11T17:13:03Z

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

Looks like we're in luck 🥳

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 ❤️

maresb · 2023-09-11T18:33:39Z

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.

Shouldn't be needed anymore after conda/conda-lock#485. Also removed some extra whitespace.

* Bump conda-lock from 1.4 to 2.3 Bumps [conda-lock](https://github.com/conda/conda-lock) from 1.4.0 to 2.3.0. - [Release notes](https://github.com/conda/conda-lock/releases) - [Commits](conda/conda-lock@v1.4.0...v2.3.0) * [condalock-command] autogenerated conda-lock files * Remove rm conda-lock.yml workaround Shouldn't be needed anymore after conda/conda-lock#485. Also removed some extra whitespace. --------- Co-authored-by: pangeo-bot <pangeo-bot@users.noreply.github.com>

itsarobin force-pushed the issue-196 branch from 3b9364f to c5c9964 Compare August 21, 2023 00:11

mfisher87 reviewed Aug 21, 2023

View reviewed changes

conda_lock/conda_lock.py Show resolved Hide resolved

mfisher87 reviewed Aug 21, 2023

View reviewed changes

itsarobin force-pushed the issue-196 branch 4 times, most recently from fb65026 to 40dc5f3 Compare August 27, 2023 22:23

mfisher87 mentioned this pull request Aug 28, 2023

Add Ruff to pre-commit (before isort + flake8 to test it as a possible replacement) #490

Merged

itsarobin force-pushed the issue-196 branch from 40dc5f3 to ed9517a Compare August 28, 2023 01:08

mfisher87 reviewed Aug 28, 2023

View reviewed changes

environments/README.md Outdated Show resolved Hide resolved

itsarobin changed the title ~~Persist packages from original lockfile for platforms not requested for lock~~ Persist packages from original lockfile _only_ for platforms not requested for lock Sep 3, 2023

mfisher87 force-pushed the issue-196 branch 5 times, most recently from 647aeca to e9a50df Compare September 3, 2023 23:14

mfisher87 and others added 6 commits September 10, 2023 10:05

Add test for updating for a single platform

c78d26e

Add unit tests for dependency removal and pip move

45b880d

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

Add documentation on correct dev environment setup

982d292

Co-authored-by: Arie Knoester <arikstr@gmail.com>

Satisfy linters

cc91ed7

Improve lock content variable names

2a1fb78

mfisher87 force-pushed the issue-196 branch from b13c677 to 2a1fb78 Compare September 10, 2023 16:06

Clarify dev dependencies README

99922db

mfisher87 reviewed Sep 10, 2023

View reviewed changes

itsarobin marked this pull request as ready for review September 10, 2023 16:36

itsarobin requested a review from a team as a code owner September 10, 2023 16:36

itsarobin requested a review from maresb September 10, 2023 16:36

mfisher87 mentioned this pull request Sep 10, 2023

conda-lock: Fix issue with removing packages from spec pymra/activities#1

Closed

mfisher87 mentioned this pull request Sep 11, 2023

Remove requirements-dev.txt #504

Merged

3 tasks

mfisher87 force-pushed the issue-196 branch from 54f685d to 99922db Compare September 11, 2023 14:29

Remove pip dev requirements instructions

4fbf8aa

maresb approved these changes Sep 11, 2023

View reviewed changes

maresb merged commit 8b3dc62 into conda:main Sep 11, 2023
11 checks passed

mfisher87 deleted the issue-196 branch September 11, 2023 19:11

mfisher87 mentioned this pull request Sep 11, 2023

Removing dependencies from an existing lock file #196

Closed

weiji14 added a commit to pangeo-data/pangeo-docker-images that referenced this pull request Sep 14, 2023

Remove rm conda-lock.yml workaround

9aad296

Shouldn't be needed anymore after conda/conda-lock#485. Also removed some extra whitespace.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

itsarobin commented Aug 20, 2023 •

edited

Loading

netlify bot commented Aug 20, 2023 •

edited

Loading

mfisher87 commented Aug 21, 2023

mfisher87 Aug 21, 2023

maresb Aug 21, 2023

mfisher87 Aug 21, 2023 •

edited

Loading

mfisher87 Sep 10, 2023

mfisher87 commented Aug 28, 2023

mfisher87 commented Sep 10, 2023

mfisher87 Sep 10, 2023 •

edited

Loading

mfisher87 Sep 10, 2023

mfisher87 commented Sep 10, 2023

maresb commented Sep 10, 2023

mfisher87 commented Sep 10, 2023

mfisher87 commented Sep 10, 2023

maresb commented Sep 11, 2023

mfisher87 commented Sep 11, 2023

mfisher87 commented Sep 11, 2023

mfisher87 commented Sep 11, 2023

maresb commented Sep 11, 2023

mfisher87 commented Sep 11, 2023 •

edited

Loading

maresb commented Sep 11, 2023

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

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

Conversation

itsarobin commented Aug 20, 2023 • edited Loading

Description

netlify bot commented Aug 20, 2023 • edited Loading

✅ Deploy Preview for conda-lock ready!

mfisher87 commented Aug 21, 2023

mfisher87 Aug 21, 2023

Choose a reason for hiding this comment

maresb Aug 21, 2023

Choose a reason for hiding this comment

mfisher87 Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

mfisher87 Sep 10, 2023

Choose a reason for hiding this comment

mfisher87 commented Aug 28, 2023

mfisher87 commented Sep 10, 2023

mfisher87 Sep 10, 2023 • edited Loading

Choose a reason for hiding this comment

mfisher87 Sep 10, 2023

Choose a reason for hiding this comment

mfisher87 commented Sep 10, 2023

maresb commented Sep 10, 2023

mfisher87 commented Sep 10, 2023

mfisher87 commented Sep 10, 2023

maresb commented Sep 11, 2023

mfisher87 commented Sep 11, 2023

mfisher87 commented Sep 11, 2023

mfisher87 commented Sep 11, 2023

maresb commented Sep 11, 2023

mfisher87 commented Sep 11, 2023 • edited Loading

maresb commented Sep 11, 2023

itsarobin commented Aug 20, 2023 •

edited

Loading

netlify bot commented Aug 20, 2023 •

edited

Loading

mfisher87 Aug 21, 2023 •

edited

Loading

mfisher87 Sep 10, 2023 •

edited

Loading

mfisher87 commented Sep 11, 2023 •

edited

Loading