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

Remove usage of pkg_resources, which has huge import overhead. #294

Merged
merged 1 commit into from
Jan 29, 2018
Merged

Remove usage of pkg_resources, which has huge import overhead. #294

merged 1 commit into from
Jan 29, 2018

Conversation

dsully
Copy link
Contributor

@dsully dsully commented Nov 20, 2017

See: pypa/setuptools#926

Providing this via #197 was a mistake. Let the caller figure it out.

@jaraco
Copy link
Owner

jaraco commented Dec 15, 2017

I'm inclined to keep this functionality. It's good practice for a library to expose its version as per the PEP. I also know there's lots of work been invested in the upstream issues to improve the performance. What is the impact of this change in your environment?

@dsully
Copy link
Contributor Author

dsully commented Dec 17, 2017

Any environment with a large number of dependencies installed in site-packages incurs a huge amount of overhead (500ms to 1s) to import pkg_resources.

Barry Warsaw & Brett Cannon are working on a core library replacement for much of the functionality of pkg_resources, but alas not the get_distribution().version API.

See: http://importlib-resources.readthedocs.io/en/latest/ for more info on that.

If you insist on providing this functionality, could you at least move it behind a function, so the import of pkg_resources is only incurred then?

@jaraco
Copy link
Owner

jaraco commented Dec 17, 2017

I'm not sure it's possible to conform to PEP 396 and continue to manage the version in package metadata and move the version resolution behind a function. Can you think of a way to make keyring.__version__ still be a property but resolved lazily?

Are you using the latest setuptools? Is it possible that recent improvements to pkg_resources have improved the situation?

I do wish to eliminate this costly startup of pkg_resources. It not only affects this use-case but also affects console scripts build by setuptools and other places where pkg_resources happens to be imported.

@dsully
Copy link
Contributor Author

dsully commented Dec 17, 2017

Internally (at LinkedIn) we've moved to generating a version.py at build time with a constant, and then simply importing that. Would that be an option here?

We're not using the absolute latest setuptools, but something pretty recent. The only way we would consider using pkg_resources voluntarily is if it did no work at import time. Right now we're actively removing our reliance on it for internal code.

And I see that Barry wrote PEP 396. Will have to have a chat with him about that. :)

@jaraco
Copy link
Owner

jaraco commented Dec 17, 2017

That's not an option here because I'm specifically seeking to indicate the version in one place (source control) and have that version reflected in the package metadata and the package's version attribute.

I guess I'd rather accept this PR than add to the burden of version management for releases.

I still hesitate because I wonder if someone might try to bring this back... or forget to after someone finds a way to remove that slow startup behavior in pkg_resources.

Can you write up a changelog entry for this change? Probably it should be a 1.x bump.

@dsully
Copy link
Contributor Author

dsully commented Dec 18, 2017

Yes, I can do that.

In general I question the value of exposing a module version at run time. PEP not withstanding.

@mitya57
Copy link
Collaborator

mitya57 commented Dec 29, 2017

I still hesitate because I wonder if someone might try to bring this back... or forget to after someone finds a way to remove that slow startup behavior in pkg_resources.

How about adding a get_version() function replacing the __version__ attribute, and adding a comment explaining why this was done?

@jaraco
Copy link
Owner

jaraco commented Jan 3, 2018

How about adding a get_version() function?

Well, mainly because it doesn't comply with the PEP. If we're not exposing the version as the __version__ attribute, then any client that needs to know the version can just get it from the package metadata (call pkg_resources directly).

@mitya57
Copy link
Collaborator

mitya57 commented Jan 4, 2018

Ok. Another solution might be using https://github.com/ionelmc/python-lazy-object-proxy or a similar wrapper. Not sure if it’s worth adding a new dependency just to get the version, though :)

@warsaw
Copy link

warsaw commented Jan 4, 2018 via email

@jaraco
Copy link
Owner

jaraco commented Jan 29, 2018

I’m thinking about a convention where the build process would put a version.txt file in the package directory. Then using importlib.resources, you access that file and parse it using semver, calver, etc.

That seems reasonable, and if the Python build tools advertised such a convention, we could use that. As it is, the package metadata, as exposed by pkg_resources, is the best, most canonical place to define and resolve the version.

I wish I had the time/energy to see if pkg_resources can't be made to limit its implicit import-time behavior, such that projects like keyring wouldn't have to make these difficult choices.

@jaraco jaraco merged commit ba7dc51 into jaraco:master Jan 29, 2018
@jaraco
Copy link
Owner

jaraco commented Jan 29, 2018

Released as 11.0.0.

@bsipocz
Copy link

bsipocz commented Apr 27, 2018

It's really unfortunate that keyring.__version__ is not available any more, it usually helps enormously when trying to pin point, or locally reproduce dependency version dependent weird test failures.

@dsully
Copy link
Contributor Author

dsully commented Apr 27, 2018

@bsipocz pip freeze | grep keyring ?

@bsipocz
Copy link

bsipocz commented Apr 27, 2018

That's not very grammatical for a list like this, when dependencies are preferably installed with conda: https://travis-ci.org/bsipocz/astroquery/jobs/370530965#L3072

@warsaw
Copy link

warsaw commented Apr 27, 2018

@jaraco I don't think it'll be fruitful to hack on pkg_resources. There's just too much baggage and backward compatibility concerns IMHO. But I would like to find a way to better support "package version runtime introspection". There are ways to do it, but it does tend to interact with build systems and such.

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.

5 participants