Skip to content

chore: uv support - #282

Open
jjfrench wants to merge 6 commits into
mainfrom
chore/uv-support
Open

chore: uv support#282
jjfrench wants to merge 6 commits into
mainfrom
chore/uv-support

Conversation

@jjfrench

@jjfrench jjfrench commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Description

  • adds uv package management
  • updates and fixes pre-commit hooks

90% of the PR is whitespace by ruff-pre-commit

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://NASA-IMPACT.github.io/veda-docs/pr-preview/pr-282/

Built to branch gh-pages at 2026-08-19 16:57 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Comment thread .pre-commit-config.yaml
@jjfrench

Copy link
Copy Markdown
Contributor Author

I recommend the Hide Whitespace setting on "Files changed" to view package management changes and ReviewNB for the notebooks.

@jjfrench
jjfrench marked this pull request as ready for review August 12, 2026 22:47
@jjfrench
jjfrench requested review from a team and wildintellect August 12, 2026 22:47
@jjfrench jjfrench self-assigned this Aug 12, 2026
Comment thread .pre-commit-config.yaml
@wildintellect

Copy link
Copy Markdown
Collaborator
  • The switch to UV, pyproject.toml seems fine.
  • The whitespace cleanup seems fine.
  • In the notebooks themselves, I've checked a few, and the changes are non-trivial. I see changes to imports and some changes to the code based on those, and I'm not sure we want to make those changes, or that they don't break the code execution (we don't have auto testing).

@jjfrench

Copy link
Copy Markdown
Contributor Author

Most of the import changes should be glob and os -> pathlib. I can comment on the other ones

@@ -446,9 +446,7 @@
],

@jjfrench jjfrench Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Line #3.            f"{RASTER_API_URL}/collections/{item['collection']}/items/{item['id']}/WebMercatorQuad/tilejson.json?&assets=cog_default&color_formula=gamma+r+1.05&colormap_name=viridis&rescale={rescale_values['min']},{rescale_values['max']}",

This requests.get was located inside a for item in items loop but didn't use item and therefore was printing the same URL x times.


Reply via ReviewNB

@@ -114,517 +114,16 @@
},

@jjfrench jjfrench Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Line #7.    cluster = gateway.connect(clusters[0].name) if clusters else GatewayCluster(shutdown_on_close=True)

functionally the same as

if clusters:
    cluster = gateway.connect(clusters[0].name)
else:
    cluster = GatewayCluster(shutdown_on_close=True)

Reply via ReviewNB

@@ -683,12 +683,10 @@
}

@jjfrench jjfrench Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Several notebooks remove the pandas import as pd is not used when import pandas as pd


Reply via ReviewNB

@@ -67,7 +67,7 @@
},

@jjfrench jjfrench Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here's an example of some of the glob/os changes made.


Reply via ReviewNB

@@ -93,21 +93,21 @@
},

@jjfrench jjfrench Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Line #3.    import contextlib

Main import change here. Uses contextlib in the case where there is a try / except -> pass

try:
    plt.colormaps.register(cmap_tornado)
except ValueError:
    pass 

with contextlib.suppress(ValueError):
    plt.colormaps.register(cmap_tornado)

Reply via ReviewNB

@@ -56,26 +56,23 @@
"outputs": [],

@jjfrench jjfrench Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the only notebook I tried fixing, to no avail (it was previously broken and remains broken). I looked at the VEDA STAC catalog, and this collection did not have COG_DEFAULT variables for rescaling. Here, we're grabbing the render parameters from the listed collection.

Variables are now corrected, but it now seems to run into issues due to image sizes


Reply via ReviewNB

@@ -81,8 +81,8 @@
"import datetime as dt\n",

@jjfrench jjfrench Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Line #6.    for _page in range(10):

page was not used, so we preface the variable with an underscore to indicate that it doesn't matter


Reply via ReviewNB

@@ -96,9 +96,7 @@
"metadata": {},

@jjfrench jjfrench Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

with (
    fs.open(SOURCE_URI) as fileobj,
    xr.open_dataset(fileobj, engine="h5netcdf") as ds,
):

Only change here is combining the two context managers into one with - contents underneath got shifted left


Reply via ReviewNB

@@ -75,29 +75,30 @@
},

@jjfrench jjfrench Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

One of the few install cells that isn't commented out - I've done this on MAAP docs before, but % is preferred to ! because % will guarantee installation in the kernel your notebook is running.


Reply via ReviewNB

@@ -71,7 +71,8 @@
"metadata": {},

@jjfrench jjfrench Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

import UTC, functionally the same as the timezone import usage of timezone.utc

datetime.now(tz=timezone.utc) -> datetime.now(tz=UTC)


Reply via ReviewNB

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Noting it's unclear if an empty newline is a qmd format requirement.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I haven't checked but I think that it applied it to every file. Does it affect qmd's?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure, we probably have to try building with quarto to see if it complains.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't see anything glaring in the build at the top of this PR

Comment thread user-guide/notebooks/datasets/nceo-biomass-statistics.ipynb
@@ -114,517 +114,16 @@
},

@jjfrench jjfrench Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Line #4.    clusters = gateway.list_clusters()

has to skip this cell but I've added back the import that was flagged as "unused". Looks like it was also missing rioxarray.


Reply via ReviewNB

@@ -48,8 +48,8 @@
"metadata": {},

@jjfrench jjfrench Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Line #5.            "query": {"datetime": {"eq": "2021-01-01T00:00:00"}},

changing this to use the top-level datatime search since it no longer goes through query


Reply via ReviewNB

@@ -67,7 +67,7 @@
},

@jjfrench jjfrench Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I had to go into EDL and accept the GESDISC DATA ARCHIVE EULA


Reply via ReviewNB

@@ -67,7 +67,7 @@
},

@jjfrench jjfrench Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

start_date = datetime.datetime(2022, 4, 1)
end_date = datetime.datetime(2022, 5, 12)

this is a ton of data - I shortened the date range when testing


Reply via ReviewNB

@@ -75,29 +75,30 @@
},

@jjfrench jjfrench Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added rioxarray


Reply via ReviewNB

@jjfrench

Copy link
Copy Markdown
Contributor Author

I completed another self-review where I ran all the notebooks on the veda hub. I've added back hvplot and rioxarray imports with a noqa tag. Fixed one STAC search cell and added a comment about EDL EULAs for another.

@wildintellect

Copy link
Copy Markdown
Collaborator

@jjfrench if you finished testing the notebooks then I'm ok with the PR, but I think we need a @NASA-IMPACT/veda-data-services review of the publication notebooks.

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.

2 participants