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

Update Poetry to 1.8.3 #637

Merged
merged 14 commits into from
Sep 3, 2024
Merged

Update Poetry to 1.8.3 #637

merged 14 commits into from
Sep 3, 2024

Conversation

romain-intel
Copy link
Contributor

Description

Replaces #636 but refactors commits to make it easier to review. Addresses #613

Still todo:

  • proper setup for the environment (needs support for environment markers to properly resolve cross platform)
  • fix the failing tests (hash override) -- I am not sure if this is still relevant as well so not 100% sure how to proceed here.
  • verification of the small code changes that were done to support the new APIs

Copy link

netlify bot commented May 7, 2024

Deploy Preview for conda-lock ready!

Name Link
🔨 Latest commit b364a27
🔍 Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/66d6bb1d63f74b000895b505
😎 Deploy Preview https://deploy-preview-637--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.

@maresb
Copy link
Contributor

maresb commented May 8, 2024

Thanks so much, the commit structure looks great!

@romain-intel
Copy link
Contributor Author

I know it's not done fully yet but let me know what you think particularly about the failing test (if I should work to fix it or if the functionality is no longer needed). I am also working on adding markers to properly be able to resolve the environment (so new feature but I guess needed to finish this).

@maresb
Copy link
Contributor

maresb commented May 15, 2024

Hi @romain-intel, it looks great so far!

The failing test was recently added in #629 so it'd be great to fix that to ensure no regression. Perhaps @sfinkens could help?

@sfinkens
Copy link
Contributor

I'll have a look (later today)

@sfinkens
Copy link
Contributor

The problem seems to be that _vendor.poetry.core.packages.package.Package.to_dependency() converts PoetryDependencyWithHash to an "ordinary" Dependency. So the hash from the environment file is lost and a new one is obtained from the online repository.

I don't know if it makes sense to just handle this extra case in to_dependency or if there's a more elegant way.

@maresb
Copy link
Contributor

maresb commented May 16, 2024

Thanks @sfinkens for the clue!

I think I've fixed it in 630adf8. Could you please review?

@maresb
Copy link
Contributor

maresb commented May 16, 2024

Looks like there's one remaining test failure: test_it_uses_pip_repositories_with_env_var_substitution[conda-response_url_without_credentials]. @jacksmith15, is this something you could help with?

@sfinkens
Copy link
Contributor

Thanks @sfinkens for the clue!

I think I've fixed it in 630adf8. Could you please review?

👍 Very elegant solution!

@romain-intel
Copy link
Contributor Author

Am back from a quick break so let me try to wrap this up. I know I still need to add environment markers (new feature but seems to be needed to deal with the deps for poetry and create the dev environments). Apart from that, are all the tests ok (I see some failures but not sure if they are flaky/expected). Let me know what else you think is needed on this and I'll get it wrapped up. Thanks for fixing the tests!!

@maresb
Copy link
Contributor

maresb commented Jun 8, 2024

Thanks so much @romain-intel for all your work on this, and I'm glad you're back!

I'm curious why environment markers are necessary.

As for what else is needed, I think we'll primarily have to lean on the test suite. Coverage is far from perfect, but hopefully this fixes more bugs than it creates.

@romain-intel
Copy link
Contributor Author

@maresb : it's because of this: 'xattr >=1.0.0,<1.1.0 ; sys_platform == "darwin"',. This is a requirement for the new poetry I included and since we use conda-lock to resolve the development environment cross platform, I thought I would try to get it to work. Of course, it's not actually functionally required and we could just have different pyproject.toml to use as sources for conda-lock for the different environments if that is also an acceptable solution (definitely a simpler one). I was trying to keep things exactly the same but happy to take other opinions (and tbh, I don't yet fully know how to pass environment markers all the way down and it may be a bit bigger change than I would like for this). If you guys are fine with just having different .toml files as sources to generate the development environments, I don't need environment markers.

For the tests, there are errors that seem to hint at a bad authorization token or something but am not sure it's related to the PR. Maybe I'll try re-running to see if that was flaky so I wasn't sure if there were actual broken tests. I'll check again.

Let me know what you think for the first thing. Happy to do the "simpler" solution.

@maresb
Copy link
Contributor

maresb commented Jun 8, 2024

Thanks for the clarification @romain-intel!

To flesh out exactly what the issue is, as I understand it, the current behavior of conda-lock is to ignore PEP 508 environment markers. We dogfood by generating lockfiles for conda-lock using conda-lock itself. The xattr dependency does not exist on Windows, so ignoring sys_platform == "darwin" breaks this locking workflow when the Windows platform is locked.

While embarrassing, I think I could tolerate breakage of the dogfood for now. It would be much nicer if we could implement a very basic version of environment specifiers, e.g. something that only supports expressions of the form sys_platform == x, and I'd rather do it at the conda-lock level rather than the Poetry level. In other words I wonder if it would make things easier if we aim for rough parity with our extremely crude selectors support.

I agree that it doesn't make much sense, but I believe there is indeed something in the Poetry upgrade that's causing the weird authorization failures, since other PRs are green, e.g. #644. I'll ping the author @jacksmith15 once again in the hopes that he's available to take a look at those auth failures.

@romain-intel
Copy link
Contributor Author

Got it, so to summarize:

  • ok to break the dogfooding by having different files to generate the envs
  • figure out the broken tests. I'll see if I can figure it out as well but any help would be welcome as well :)

@maresb
Copy link
Contributor

maresb commented Jun 10, 2024

by having different files to generate the envs

I suspect there's a miscommunication here. Wouldn't having different envs work around the dogfooding breakage? I'm suggesting to simply break it.

Anyways, feel free to do whatever makes sense to you. I have the highest trust in your judgement.

@romain-intel
Copy link
Contributor Author

That's a dangerous proposition :).

At any rate, it seems the only actual failure is this: _ test_it_uses_pip_repositories_with_env_var_substitution[conda-response_url_without_credentials] _. I'll check around and fix it and also update the branch. Am going to try to finish this this week to get it ready :)

@romain-intel
Copy link
Contributor Author

This is really weird. Running that test independently on my mac works fine. Let me keep digging.

@maresb
Copy link
Contributor

maresb commented Jul 24, 2024

@romain-intel, thanks so much for your persistence!!!

I believe Poetry's cache was preventing you from being able to reproduce the error locally, so I modified the test to clear the cache. This allowed me to drill down with a debugger. While it was hard to see exactly what was going on, what was clear is that Poetry wants to handle basic auth through the config dictionary, not the URL.

Upon stripping the creds from the URL and passing them instead via the config dict, the tests pass.

Would you (or anyone else tracking this) be able to critically review my changes?

As for an initial self-review:

  1. The config dictionary is mutated by the config.merge method. I don't understand all the Poetry internals to know for sure if each Poetry repository gets a reference to the same config or if there's a copy operation at some point. I think this won't matter in the end though, as long as each repository knows its own credentials.
  2. Along these lines, I don't think we have any tests for multiple different passwords corresponding to multiple pip repositories at the same time. Probably we should be testing this, but it seems a bit out-of-scope here.
  3. Due to the need for Poetry's cache to be cleared, since we're using pytest-xdist, ideally use a locking mechanism (e.g. a lockfile... of the other type 😂) to prevent the multiple parameter configurations of test_it_uses_pip_repositories_with_env_var_substitution from executing simultaneously. There are a few suggestions in this direction on this page but there's still some work required in order to achieve what I described, though it doesn't seem difficult.

@romain-intel
Copy link
Contributor Author

Ah ha! The cache. Of course the cache. Thank you thank you thank you. OK, I'll take a look at the code changes and see if I can add what you describe in your self review. Progress!!!

Copy link
Contributor Author

@romain-intel romain-intel left a comment

Choose a reason for hiding this comment

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

Changes look nice :)

return urlunparse(base_url._replace(netloc=base_url.netloc.split("@", 1)[-1]))
return stripped_url(self.base_url)

@property
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: stripped_base_url and stripped_url are the same thing. Do we need/want both?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think they are the same. Should have added example values. 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh right, sorry, I stupidely missed base_url vs url in the arg :(

with FileLock(to_delete.parent / ".conda_lock.lock"):
yield
# Do another independent check that triggers even if we're in optimized mode
if "pypoetry-conda-lock" in to_delete.parts:
Copy link
Contributor

@maresb maresb Aug 7, 2024

Choose a reason for hiding this comment

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

Oh, there's a risk of silent failure if this condition is false. We should fail hard.

Also, what happens if the lockfile doesn't get cleaned up? Will the tests just silently hang waiting for release?

Thanks @romain-intel!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll fail hard. Let me see if I can get something to clean it up better. I suspect they may hang but maybe what we can do is fail hard too (a timeout there) and I can maybe introduce a general cleanup at the end of the whole test.

@romain-intel
Copy link
Contributor Author

Since I am so slow, also updated to 1.8.3. Hopefully this is it for this :).

@maresb
Copy link
Contributor

maresb commented Sep 2, 2024

Thanks so much @romain-intel!!! I'm slow too 🙈

What do you think of this?
romain-intel#2

Can you think of any blockers? It'd be nice to get this merged.

One last re-request in terms of auditability would be to rebase so that we have just a single commit that runs nothing other than vendoring.

@maresb
Copy link
Contributor

maresb commented Sep 2, 2024

I don't really have time to track it down at the moment, but frustratingly I can't run pytest locally because I get:

conda_lock/_vendor/poetry/installation/wheel_installer.py:10: in <module>
    from installer import install
E   ModuleNotFoundError: No module named 'installer'

At least it works on the CI

@romain-intel romain-intel force-pushed the feat/poetry-1.8.2_1 branch 2 times, most recently from a2e25ec to 849306c Compare September 3, 2024 07:18
@romain-intel
Copy link
Contributor Author

OK, I think I made both changes. It is indeed much cleaner :). Let me know what you think.

@romain-intel romain-intel changed the title [WIP] Feat/poetry 1.8.2 1 Update Poetry to 1.8.3 Sep 3, 2024
@maresb
Copy link
Contributor

maresb commented Sep 3, 2024

Incredible @romain-intel!

I audited f43e218 because that's way too much to look at.

I reproduced it exactly with the following commands:

vendoring sync -vvv .
dos2unix conda_lock/_vendor/poetry/core/_vendor/lark/grammars/*
dos2unix conda_lock/_vendor/poetry/core/_vendor/fastjsonschema/*
dos2unix conda_lock/_vendor/conda/_vendor/boltons/LICENSE
dos2unix conda_lock/_vendor/poetry/core/_vendor/lark/LICENSE

I honestly didn't think this was feasible, but incredibly it seems to me like we're done! Thanks so much!!! ❤️

@maresb maresb merged commit c1030fb into conda:main Sep 3, 2024
10 checks passed
@romain-intel
Copy link
Contributor Author

whoohoo!! Thanks for merging!!!

@maresb maresb mentioned this pull request Sep 5, 2024
2 tasks
romain-intel added a commit to romain-intel/conda-lock that referenced this pull request Sep 6, 2024
PR conda#637 initially vendored 1.8.2 and dependencies had been updated for that.
It then included 1.8.3 which changed some dependencies.

This updates the dependencies for 1.8.3. It also fixes an issue with
~= versus = ^ (and therefore relaxes the previous dependencies.

Also regenerated the environments/conda-lock.yml file as a result
of the modifications on pyproject.toml using
`conda-lock --platform linux-64 --platform osx-64 --platform osx-arm64 --platform linux-aarch64 --file=environments/dev-environment.yaml --file=pyproject.toml --lockfile=environments/conda-lock.yml`
@maresb
Copy link
Contributor

maresb commented Sep 8, 2024

Important reference: the version pins here are incorrect and have been fixed in #678.

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

Successfully merging this pull request may close these issues.

3 participants