-
Notifications
You must be signed in to change notification settings - Fork 84
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
Some binder and conda env tweaks #821
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #821 +/- ##
=======================================
Coverage ? 73.64%
=======================================
Files ? 31
Lines ? 1996
Branches ? 0
=======================================
Hits ? 1470
Misses ? 526
Partials ? 0 ☔ View full report in Codecov by Sentry. |
…ooks don't import anything but earthaccess and the standard library, and most of these deps are in the docs tutorials notebooks so would be available in the development environment
c56b7a7
to
01f307b
Compare
I'm 1000% on board with this. I share this exact preference. I even have a bash alias that activates an environment matching the name of the current directory. I |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice :)
- `binder/` directory as we no longer need a special [binder](https://mybinder.org) | ||
environment with the top-level `environment.yml` introduce in | ||
[#733](https://github.com/nsidc/earthaccess/issues/733) | ||
([@jhkennedy](https://github.com/jhkennedy)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel too strongly about this, but I'd be OK omitting this from the change log. It's not really user facing. For developers, we have PRs and commits as history, so I don't feel we need things like this in changelog.
Co-authored-by: Chuck Daniels <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
When reviewing #733, we had a short discussion on the
binder/
directory and environment files within, and whether the extra environment or dependencies were needed anymore, and we decided to circle back to it, which I've done here.This PR removes the binder directory and environment file within. I've done this because:
environment.yml
file and works just fine; here's a link to a binder instance of this PR's branch and a screenshot of it running:https://mybinder.org/v2/gh/jhkennedy/earthdata/binder-conda-tweaks
binder/environment.yml
are either not used or already in thedocs
dependency group which gets installed in the top-level environment file. (inventoried here: 65b0998)I've also made two tweaks to the top-level
environment.yml
:uv
does not restrict the python version outside of thepython-requires
in thepyproject.toml
and this will better matchuv
's behavior (if we do want a default version, we should use a.python-version
file).earthaccess
fromearthaccess-dev
(bolded b/c this might be the most controversial). Generally, this is my preference -- the environment matches the project name. I believe the-dev
is a legacy of having multiple environment files, which we no longer do as of Replacepoetry
withpip
, build withhatchling
, dynamically calculate minimum dependencies withuv
#733 (except the binder one, which I've removed in this PR). I think this should be fine as we previously hid the conda environment file in theci/
directory (although we did document it as an alternative to poetry). In our current docs we don't explicitly state the environment name, and since we haven't released Replacepoetry
withpip
, build withhatchling
, dynamically calculate minimum dependencies withuv
#733, I think there should be very little downstream impact on users outside of the developers who prefer conda.(I can expand on this preference if there is a question about, or pushback to, this change. )
Pull Request (PR) draft checklist - click to expand
contributing documentation
before getting started.
Ensure an issue exists representing the problem being solved in this PR.title such as "Add testing details to the contributor section of the README".
Example PRs: #763
example
closes #1
. SeeGitHub docs - Linking a pull request to an issue.
CHANGELOG.md
with details about your change in a section titled## Unreleased
. If such a section does not exist, please create one. FollowCommon Changelog for your additions.
Example PRs: #763
README.md
with details of changes to theearthaccess interface, if any. Consider new environment variables, function names,
decorators, etc.
Click the "Ready for review" button at the bottom of the "Conversation" tab in GitHub
once these requirements are fulfilled. Don't worry if you see any test failures in
GitHub at this point!
Pull Request (PR) merge checklist - click to expand
Please do your best to complete these requirements! If you need help with any of these
requirements, you can ping the
@nsidc/earthaccess-support
team in a comment and wewill help you out!
Request containing "pre-commit.ci autofix" to automate this.
📚 Documentation preview 📚: https://earthaccess--821.org.readthedocs.build/en/821/