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

Update version detection #3364

Merged
merged 1 commit into from
Feb 21, 2024
Merged

Conversation

dopplershift
Copy link
Member

Description Of Changes

Try to avoid pulling in setuptools_scm for better import time by directly checking for a pip file that indicates whether the install is editable and only using the git-based version in that case; otherwise, depend on the distribution metadata.

Alternative to #3363.

@dopplershift dopplershift added Area: Infrastructure Pertains to project infrastructure (e.g. CI, linting) Type: Maintenance Updates and clean ups (but not wrong) labels Jan 18, 2024
@dopplershift dopplershift requested a review from a team as a code owner January 18, 2024 01:56
@dopplershift dopplershift requested review from dcamron and removed request for a team January 18, 2024 01:56
@dopplershift
Copy link
Member Author

@akrherz Does this accomplish the same optimization you saw in #3363?

@akrherz
Copy link
Contributor

akrherz commented Jan 18, 2024

Wow @dopplershift, there's python foo I have never seen before here, ufff. Indeed, testing this

import metpy
check main this PR
timing 1.4s 1.1s
memory (memray) 92.2 MB 88MB

Try to avoid pulling in setuptools_scm for better import time by
directly checking for a pip file that indicates whether the install is
editable and only using the git-based version in that case; otherwise,
depend on the distribution metadata.
@dopplershift
Copy link
Member Author

Regarding tests and coverage, I believe our thinking in the past was that we make sure CI covers the path for the released code, and our dev installs will cover...well the dev path. I don't think there's a good way to test this that doesn't essentially mock things out based on the implementation or require another CI job that uses a dev-style (editable) install.

@dopplershift
Copy link
Member Author

@dcamron We can ignore the coverage failure here, so this is ready for review.

Copy link
Member

@dcamron dcamron left a comment

Choose a reason for hiding this comment

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

Sounds good. I'd meant to make room for the testing/coverage discussion, but I'm happy with your summary above. 👍

@dopplershift
Copy link
Member Author

The important part is that we don't release a broken package, and I'm confident we're testing for that.

@dopplershift dopplershift merged commit 8ccabb7 into Unidata:main Feb 21, 2024
33 of 34 checks passed
@dopplershift dopplershift deleted the version-finding branch February 21, 2024 21:55
@github-actions github-actions bot added this to the 1.7.0 milestone Feb 21, 2024
@dopplershift dopplershift modified the milestones: 1.7.0, 1.6.2 Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Infrastructure Pertains to project infrastructure (e.g. CI, linting) Type: Maintenance Updates and clean ups (but not wrong)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants