conda-forge / filesystem-spec-feedstock

A conda-smithy repository for filesystem-spec.
BSD 3-Clause "New" or "Revised" License
2 stars 9 forks source link

Improve importlib-metadata handling #48

Closed zklaus closed 3 years ago

zklaus commented 3 years ago

Checklist

Fixes #42

The previous attempt at addressing #42, #45, was only partially successful (see https://github.com/conda-forge/filesystem-spec-feedstock/issues/42#issuecomment-819654926). This PR tries to address this with a more comprehensive patch.

conda-forge-linter commented 3 years ago

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.

zklaus commented 3 years ago

@conda-forge-admin, please rerender

github-actions[bot] commented 3 years ago

Hi! This is the friendly automated conda-forge-webservice. I tried to rerender for you, but it looks like there was nothing to do.

martindurant commented 3 years ago

I am happy to merge this, but I've never done this kind of thing, so I don't know for sure that this will work. Do we need a second opinion?

zklaus commented 3 years ago

I would certainly happy for another opinion. @TomAugspurger, would you like to weigh in? Or perhaps @ocefpaf?

TomAugspurger commented 3 years ago

I'm not familiar with importlib-metadata.

zklaus commented 3 years ago

I think the question is more: Is patching an sdist tarball in this form appropriate, not necessarily anything specific about importlib-metadata.

maresb commented 3 years ago

I hope I'm not out of line here... maybe I don't understand people's motivations or constraints, so please disregard if I'm saying something nonsensical here.

@zklaus, to be honest I don't understand your rush. A new version will be coming sometime soon, and that should fix things. Once that version drops, you'd have to remove these patches anyway, right? Maybe you have a valid reason to push this through immediately, but it seems to me like having patience would allow the maintainers to concentrate on what they need to get done for their release.

I'm not a patient person myself, so I'd be hypocritical to seriously preach patience. In case it helps, here is what I did for my situation:

I've been working on a recipe depending on this feedstock. At first I was frustrated by being blocked, but then realized there's a simple workaround: just add importlib-metadata as a fake dependency. I even added a pull request to remove the fake dependency once this gets fixed. Now I can comfortably wait. Assuming your situation is similar, would this idea help you?

zklaus commented 3 years ago

@maresb, don't worry about lines, none were crossed as far as I am concerned.

Otherwise, fair question, and I have no big problem with things taking a bit of time. But here is where I am coming from:

My interest came because a user of our project made a fresh install of said project and only got an exception right out the gate. Our project here depends on fsspec, but at least twice removed. If only I had the problem, I would install importlib-metadata locally and forget about it. Like this, we could still just install importlib-metadata as another dependency in our own package, but keep in mind that that is then a required workaround for a potentially large number of packages. And all of those packages need to remove this workaround sometime in the future. So from this perspective, I think it's definitely worthwhile fixing the issue here and/or upstream and it becomes a question of weighing costs and benefits.

So what are the costs? There was the investigation of the issue, of course. This was unavoidable and the conclusion seems to be that there will be an upstream fix for the next version. Given the track record of the project, this release can be expected within the next two weeks or so with rather high confidence.

I have to confess that I did not estimate this time frame and it is really close.

In the general case, however, only going off the maintainer's promise of "shortly releasing a new version", that time frame can be surprisingly long; it certainly can be when I am the maintainer.

What about the benefits? Of course, we don't need to wait for the next release, so if the install of my own package works more smoothly for new users, that's definitely a plus for me. But on top of that we also get another release fixed. If the next release turns out to be incompatible, we are still good.

There are ancillary benefits from my point of view: I get to learn how conda-forge figures dependencies and handles patches and I can cross this issue (which as I said is a very real concern for new users of my own project) off my list.

But if the decision is to relegate this issue to the next version, no problem.

maresb commented 3 years ago

Thanks for explaining @zklaus, everything you say makes a lot of sense. I hadn't realized how widely used this package is.

Patching the sdist still feels a bit extreme to me, but given @martindurant's vehemence against dependencies, and since you've already done the work, it looks good to me!

martindurant commented 3 years ago

Thank you for the remarkably clear thoughts, @zklaus Are we agreed to merge this?

By "soon", I meant today, maybe Monday. s3fs and gcsfs will follow within 24hr after conda-forge is done with fsspec, but I ask you all to have a careful look at the setup.py, requirements.txt and feedstock when the process is rolling!

zklaus commented 3 years ago

I would be in favor of merging, but your call either way.

maresb commented 3 years ago

Either way is good by me too!

@martindurant my quick test of cloning your master branch, running pip install and pip check in a clean environment succeeded, so as far as I can tell you have solved this particular issue.

The feedstock looks good to me. We'll have to remove the patch once the release comes out. That will be very easy: an automated PR will be made, and we just modify it there.

martindurant commented 3 years ago

OK, merging. Sorry for causing work for everyone!

zklaus commented 3 years ago

No worries! Learning about optional dependencies has been on my agenda for a while :smile:

maresb commented 3 years ago

Success downstream!!! Thanks everyone!

zklaus commented 3 years ago

Cheers :smile: