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

HARMONY-1862: Update pystac library dependency #47

Merged
merged 8 commits into from
Sep 27, 2024
Merged

Conversation

vinnyinverso
Copy link
Contributor

@vinnyinverso vinnyinverso commented Sep 24, 2024

Jira Issue ID

HARMONY-1862

Description

This updates pystac compatibility by allowing you to use newer versions of pystac. This should allow you to use the latest version of pystac if using python 3.9 or 3.10.

Local Test Steps

Test a service (with local Harmony deployment):

  1. Checkout branch HARMONY-1862 of this repo
  2. If testing with harmony service example, also checkout branch HARMONY-1862 of the harmony service example repo
  3. Build a service image using this branch of the service lib (see the service example README for instructions on building a service with a local copy/branch of the service lib).
  4. Update your local Harmony instance service deployment to use the newly built image.
  5. Run an example request and make sure it succeeds.

Standalone test:

  1. Activate your virtual environment (python 3.9 or 3.10).
  2. Run make install
  3. Specify a newer version of pystac in your virtual environment (e.g. pip install pystac==1.10.1).
  4. Run make test.

PR Acceptance Checklist

  • Acceptance criteria met
  • Tests added/updated (if needed) and passing
  • Documentation updated (if needed)

@@ -1,7 +1,7 @@
autopep8 ~= 1.5
debugpy ~= 1.2
Faker ~= 8.1.3
flake8 ~= 5.0.4
flake8 >= 6.1.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this because flake8 was mistaking the colon in http:// for a JSON-like colon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe should have an upper bound here? I want it to be flexible though because of all of the python versions we support.

Copy link
Contributor Author

@vinnyinverso vinnyinverso Sep 24, 2024

Choose a reason for hiding this comment

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

Maybe should avoid the upper bound according to this:
https://iscinumpy.dev/post/bound-version-constraints/#tldr

Only add a cap if a dependency is known to be incompatible or there is a high (>75%) chance of it being incompatible in its next release. Do not cap by default - capping dependencies makes your software incompatible with other libraries that also have strict lower limits on dependencies, and limits future fixes. Anyone can fix a missing cap, but users cannot fix an over restrictive cap causing solver errors. It also encourages hiding issues until they become harder to fix, it does not scale to larger systems, it limits your ability to access security and bugfix updates, and some tools (Poetry) force these bad decisions on your downstream users if you make them.

related: nasa/harmony-py#79

Makefile Outdated
@@ -36,4 +36,4 @@ test-no-warnings:
pytest --disable-warnings --cov=harmony tests

cve-check:
safety check
safety check -i 72236
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just temporary until we move away from setuptools.

requirements.txt Outdated
@@ -1,7 +1,7 @@
boto3 ~= 1.14
deprecation ~= 2.1.0
pynacl ~= 1.4
pystac ~= 0.5.3
pystac >= 0.5.3
Copy link
Contributor Author

@vinnyinverso vinnyinverso Sep 24, 2024

Choose a reason for hiding this comment

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

The latest version of pystac is not compatible with python 3.8, so we need some kind of range here to continue supporting python 3.8, 3.9 and 3.10.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I originally had this pinned to the latest pystac version but realized it wouldn't work with python 3.8)

Copy link
Contributor Author

@vinnyinverso vinnyinverso Sep 24, 2024

Choose a reason for hiding this comment

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

Not having an upper bound is consistent with the guidance from the article referenced here https://github.com/nasa/harmony-service-lib-py/pull/47/files#r1773854232.

@vinnyinverso vinnyinverso changed the title HARMONY-1862: Update STAC IO library HARMONY-1862: Update pystac library dependency Sep 24, 2024
@vinnyinverso vinnyinverso marked this pull request as draft September 24, 2024 19:30
@vinnyinverso vinnyinverso marked this pull request as ready for review September 25, 2024 11:44
@vinnyinverso vinnyinverso marked this pull request as draft September 25, 2024 17:48
@vinnyinverso vinnyinverso marked this pull request as ready for review September 25, 2024 21:52
@ygliuvt
Copy link
Member

ygliuvt commented Sep 26, 2024

if the pystac version (>= 1.0.0) is incompatible with python 3.8, we should update the README and code comments to remove python 3.8 from supported python versions.

@vinnyinverso
Copy link
Contributor Author

if the pystac version (>= 1.0.0) is incompatible with python 3.8, we should update the README and code comments to remove python 3.8 from supported python versions.

It should be compatible. Sorry if that wasn't clear.

Copy link
Contributor

@chris-durbin chris-durbin left a comment

Choose a reason for hiding this comment

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

Tested out successfully after building the harmony-service-example image from the HARMONY-1862 branch.

.gitignore Outdated
@@ -132,3 +132,6 @@ config.json

# Snyk / Deepcode AI
.dccache

# VS Code
.vscode/settings.json
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the whole .vscode directory would be good

Copy link
Member

@ygliuvt ygliuvt left a comment

Choose a reason for hiding this comment

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

Tested successfully locally.

@chris-durbin chris-durbin merged commit 2ebef48 into main Sep 27, 2024
8 checks passed
@chris-durbin chris-durbin deleted the HARMONY-1862 branch September 27, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants