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

Make it more robust #7

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

flying-sheep
Copy link

If this is robust against hatchling not being installed, it should also be robust against hatchling being installed without hatch-vcs. Maybe someone uses the library this code is in inside of some other hatch plugin or so!

Similarly the LookupError probably occurs on CI or a production environment or so where installing or configuring git just to have this not crash would feel like a hassle, so we should catch that too.

I think the other problems are more genuine user errors that we shouldn’t protect against.

@maresb
Copy link
Owner

maresb commented Jan 17, 2025

It's an honor to receive your visit, @flying-sheep!

I was actually agonizing about this particular choice a few weeks ago when I did the update. The thing I don't like about this proposal is that it if someone has a dev environment where at some point either git or hatch-vcs is uninstalled, then the dynamic update will silently fail. My sense is that it's more useful to fail loudly in such situations.

The other idea I was considering was to make a PyPI package and do full error handling. I decided against that for now because I don't want to maintain more stuff, and I don't like the idea of adding yet another dependency to do this hack. (Although some people may find it useful.)

I don't feel so strongly either way, but I'd appreciate the discussion and feedback. Thanks so much for your interest!

@flying-sheep
Copy link
Author

It's an honor to receive your visit, @flying-sheep!

Haha thank you very much, can’t say I’ve read that before ☺️

This repo is very useful, thanks for maintaining it!

The thing I don't like about this proposal is that it if someone has a dev environment where at some point either git or hatch-vcs is uninstalled, then the dynamic update will silently fail. My sense is that it's more useful to fail loudly in such situations.

I totally get that. I’ve been burned by silent failure in the past as well. The only way to fix that would be to add debug logging.

My thought was production environments: If someone happens to have hatchling installed, but not hatch-vcs, it causes a crash. Say someone uses this repo’s pattern in their library. Then a hatchling plugin depends on that library. Then someone uses that hatchling plugin. Suddenly the library will crash when trying to build with the hatchling plugin. And there’s no way to circumvent that other than needlessly depending on hatch-vcs also.

@maresb
Copy link
Owner

maresb commented Jan 18, 2025

Haha thank you very much, can’t say I’ve read that before ☺️

I've definitely seen you around a lot before. Either we hang out in similar GitHub circles, or you're one of these omnipresent GitHub figures. 🙂

Say someone uses this repo’s pattern in their library.

Hmm, I really hope that nobody would use this pattern in their library! 😂 Maybe I didn't emphasize the "footgun" aspect enough.

The particular context I originally developed this for was consulting projects where I'm on a team developing a webservice, and we're also responsible for the deployment. We had a dev environment where the webserver was live and autoreloading, so it was really useful that people could hop on and live code, and so we also needed to be able to check the version and commit we were on without a full reinstall.

But if you really wanted to use this in a production context, I think the semi-proper way to do so would be something like

__version__ = _get_importlib_metadata_version()
if os.environ.get("MYPACKAGE_DEV"):
    __version__ = _get_hatch_version()

This way we only trigger hatch-vcs when the developer explicitly sets an environment variable. The downside is that we never check the output of _get_importlib_metadata_version in the development environment, but if you care, then hopefully you also have a staging environment to catch this sort of thing.

Maybe it would even make sense for this envvar approach to be our default recommendation? (In this case we can remove catching the import error so that __version__ is never None.

My thought was production environments: If someone happens to have hatchling installed, but not hatch-vcs, it causes a crash.

Doesn't it just fail with the error Unknown version source: vcs? I think the error text in hatchling could be improved to make it more informative and less confusing, but at least you can clearly see what's wrong, right? Is it correct that the true underlying issue you're concerned about is how to prevent this from triggering in a non-dev context?

Part of the philosophy I had was to make it a simple code snippet for which a developer can easily assume personal responsibility for. Catching errors feels to me a bit like shifting the responsibility. But as I said, I don't feel so strongly.

Thanks a lot for the discussion, I really appreciate your feedback!

@flying-sheep
Copy link
Author

flying-sheep commented Jan 18, 2025

Either we hang out in similar GitHub circles, or you're one of these omnipresent GitHub figures. 🙂

a little bit of column A, a little bit of column B. I spend a lot of time with packaging and data science (and its enabling technology like linear algebra, array storage, and so on), but I generally tend to submit PRs to everything I use when there’s an annoyance that I think I can fix in a reasonable timeframe!

Hmm, I really hope that nobody would use this pattern in their library! 😂

I agree: people should make their dev workflow re-install editable packages to update the metadata, or there should be support for dynamic package version metadata at runtime. But since there are no convenient ways to do the former and no progress on the latter, some people (like my co-maintainers) prefer using __version__ to standard metadata, because it can be made to reliably represent the currently checked-out version with this pattern (i.e. you git switch v1.0.5 and __version__ == '0.1.5' even if you pip install -e’d it last time from the main branch)

But if you really wanted to use this in a production context, I think the semi-proper way to do so would be something like […]

Good idea! @ilan-gold would that work for you? Then we can get rid of the hackery.

Doesn't it just fail with the error Unknown version source: vcs?

yeah, but my point is that it does so in this scenario:

  1. footgun is an (e.g. utility) package using the pattern
  2. hatchling-coolstuff is a hatchling plugin depending on footgun
  3. mypkg has build-system.requires = ["hatchling", "hatchling-coolstuff"]

now building mypkg fails because hatchling is installed while hatch-vcs isn’t. mypkg doesn’t need hatch-vcs, it’s just forced to add it to its dependencies because of an implementation detail of footgun

Thanks a lot for the discussion, I really appreciate your feedback!

likewise!

@maresb
Copy link
Owner

maresb commented Jan 18, 2025

Ya, I see your point now. Thanks for spelling it out in detail.

The environment variable solution would also resolve your point, wouldn't it? If so, I'd be inclined to go that route.

maresb added a commit that referenced this pull request Jan 19, 2025
maresb added a commit that referenced this pull request Jan 19, 2025
@ilan-gold
Copy link

Good idea! @ilan-gold would that work for you? Then we can get rid of the hackery.

I would be good with an env variable - how would it be set?

@flying-sheep
Copy link
Author

You can e.g. set it in your shell rc (.zshrc/.bashrc/…), or set it in the tool you use to define your environment, e.g.:

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