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

Revert pinning of importlib-metadata #54

Merged
merged 3 commits into from Mar 8, 2022

Conversation

dhirschfeld
Copy link
Member

@dhirschfeld dhirschfeld commented Feb 20, 2022

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@dhirschfeld
Copy link
Member Author

@conda-forge-admin, please rerender

@dhirschfeld
Copy link
Member Author

This PR reverts #53. In that PR importlib-metadata was pinned to align with upstream:
https://github.com/PyCQA/flake8/blob/8c1ed24738ca6ed0fa8a76f8890d104e63878323/setup.cfg#L45

This enables the use of pip check in downstream recipes however the pinning makes flake8 impossible to install into many new environments and that problem will only get worse as more packages depend on newer importlib-metadata versions.

Whilst I would normally advocate following upstream pinnings, in this case the pinning is there solely to avoid a deprecation warning. I think it's far preferable for users to be able to install flake8 and experience a deprecation warning than it is for them to be unable to install any package which depends on flake8.

flake8 isn't in my environment spec but one of the packages I need depends on it and another depends on a modern importlib-metadata. Because mamba strictly enforces dependency specifications I'm unable to create the environment all because of a deprecation warning.

I think the upstream pinning is inappropriate and shouldn't be followed in this case. Downstream packages will simply have to not use pip check. There are multiple instances where pip check doesn't work for conda packages so I don't think we should be doggedly using it - certainly not where it makes environments impossible to solve.

Copy link
Member

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

I'm ok with it since it seems the least terrible of no good options.

@dhirschfeld
Copy link
Member Author

Thanks @dopplershift. I just spent 30mins tracking down a failed install of spyder which came down to this issue.

@dhirschfeld
Copy link
Member Author

I'm ok with it since it seems the least terrible of no good options.

@conda-forge/flake8 any objections? If not, I'd love to get this merged so I can update my environment.

@dhirschfeld
Copy link
Member Author

ping @conda-foge/core - could someone please merge this as it is blocking a lot of environments from being updated.

@dopplershift dopplershift merged commit 3cafbde into conda-forge:master Mar 8, 2022
@dhirschfeld
Copy link
Member Author

Thanks @dopplershift!

@dhirschfeld dhirschfeld deleted the patch-1 branch March 8, 2022 23:13
@ccordoba12
Copy link

ccordoba12 commented Mar 31, 2022

Why was this accepted and merged? After fixing it in conda-forge/admin-requests#375, I saw it resurfaced and I spent two hours tracking the problem down again.

I think the upstream pinning is inappropriate and shouldn't be followed in this case. Downstream packages will simply have to not use pip check

I disagree @dhirschfeld. This is really useful to detect bad packaging everywhere and we use it in a lot of feedstocks we maintain.

@ccordoba12
Copy link

I think the real solution for this is to make flake8 a regular package (i.e. no noarch), so that the dependency on importlib_metadata can be correctly declared for Python 3.7

@conda-forge/core, thoughts about this?

@ccordoba12
Copy link

flake8 isn't in my environment spec but one of the packages I need depends on it and another depends on a modern importlib-metadata. Because mamba strictly enforces dependency specifications I'm unable to create the environment all because of a deprecation warning.

I'm sorry but this is awful reasoning. We maintainers work hard to keep conda-forge working the best we can for users to come and change things to their convenience.

If it was not possible to install two packages with conflicting importlib_metadata, you could have installed installed Spyder in another environment and connect it to the first one by following these instructions (which you probably know):

http://docs.spyder-ide.org/current/faq.html#using-existing-environment

@dopplershift
Copy link
Member

dopplershift commented Apr 1, 2022

@ccordoba12 So the answer to your original question is:

This enables the use of pip check in downstream recipes however the pinning makes flake8 impossible to install into many new environments and that problem will only get worse as more packages depend on newer importlib-metadata versions.

I fundamentally disagree with this assertion:

This is really useful to detect bad packaging everywhere and we use it in a lot of feedstocks we maintain.

Quite frankly I've seen it cause more problems than it solves. I accept that's just my experience, but I don't find it to be some panacea for our dependency issues. Tools exist to serve us, we don't exist to serve the tools.

I'm sorry but this is awful reasoning. We maintainers work hard to keep conda-forge working the best we can for users to come and change things to their convenience.

Yes, and this maintainer made the call, after more than 2 weeks of no comment and no involvement by anyone else on this repo, and a lifetime of dealing with user frustration that comes with package conflicts of all sorts, that providing users a way to actually move forward and get new versions installed easily was, AS I SAID, "the least terrible of no good options" (Clearly your users must be comfortable using multiple environments, but many many many more aren't--and multiple envs aren't always even a solution). This was IMO how we could

keep conda-forge working the best we can for users

I get that it's frustrating having something break again on you, and I apologize that I missed that you fixed this before on the admin-requests repo. I did take the time to review #52 and #53 which argued for and implemented the pinning before merging this PR, and your use case was not among the arguments made there. I missed the initial discussion about pip check.

I think calling that reasoning "awful" is completely uncalled for--and the criticism rings shallow when you haven't signed up to help take the responsibility of maintaining this recipe.

All that said, I agree that making flake8 non-noarch until we stop building Python 3.7 packages is a reasonable path forward to address all of the needs discussed here. I'd be happy to review and merge a PR doing so.

@ocefpaf
Copy link
Member

ocefpaf commented Apr 1, 2022

Tools exist to serve us, we don't exist to serve the tools.

Okay. I am making a t-shirt with this for the next scipy. Who wants one? I'm buying!

@dopplershift
Copy link
Member

dopplershift commented Apr 1, 2022

Okay. I am making a t-shirt with this for the next scipy. Who wants one? I'm buying!

I would be willing to spend many 💵 on such a shirt.

I also just want to point out how inane the chain of events here is. We are likely going to be building arch- and python version-specific packages (22 28 CI builds in total currently) of a PURE PYTHON tool due to a version pinning so that another tool doesn't complain. A version pinning that exists for no reason other than to avoid a deprecation warning, no actual breakage. 28 separate packages, 28 CI machine creations (burning electricity) all to produce identical collections of Python code, with the only variation being whether they depend on importlib-metadata. To avoid a deprecation warning.

@ccordoba12
Copy link

I think calling that reasoning "awful" is completely uncalled for--and the criticism rings shallow when you haven't signed up to help take the responsibility of maintaining this recipe.

I agree with you and I accept my responsibility on that. After conda-forge/admin-requests#375 was merged, I saw no reason for becoming a maintainer here since the problem was solved for me, but I should have gone one step further.

Okay. I am making a t-shirt with this for the next scipy. Who wants one? I'm buying!

Haha, I think I pre-ordered at least a dozen already!

Sorry @dopplershift for venting my frustration here, but these little things sometimes take way too much of my time.

I also just want to point out how inane the chain of events here is. We are likely going to be building arch- and python version-specific packages (22 28 CI builds in total currently) of a PURE PYTHON tool due to a version pinning so that another tool doesn't complain.

I understand your point, but if I'm not mistaken, this is not only an issue with pip check but it also has to do with avoiding pkg_resources.VersionConflict errors. In Spyder's case, those errors usually break our code completion and linting facilities.

@dhirschfeld
Copy link
Member Author

Maybe I'm being dense, but if spyder wants to specify an environment that will pass pip check can't spyder pin importlib-metadata <4.3 themselves?

That would still allow others to install newer versions of importlib-metadata alongside flake8 and would allow pip check to pass for spyders CI, if having pip check work is important to them.

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.

None yet

5 participants