search

conda / conda-lock

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

[WIP] Feat/poetry 1.8.2 1 #637

Open romain-intel opened 1 month ago

romain-intel commented 1 month ago

Description

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

Still todo:

netlify[bot] commented 1 month ago

Deploy Preview for conda-lock ready!

Name Link
Latest commit 630adf89bf96f96575fcbc737b7a787538b8be6a
Latest deploy log https://app.netlify.com/sites/conda-lock/deploys/6645dadaed7533000850fed7
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 commented 1 month ago

Thanks so much, the commit structure looks great!

romain-intel commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

I'll have a look (later today)

sfinkens commented 1 month ago

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 commented 1 month ago

Thanks @sfinkens for the clue!

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

maresb commented 1 month ago

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 commented 1 month ago

Thanks @sfinkens for the clue!

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

:+1: Very elegant solution!

romain-intel commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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. https://github.com/conda/conda-lock/pull/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 commented 3 weeks ago

Got it, so to summarize:

maresb commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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