Test nim inference workflow and make minor updates#34
Conversation
|
/blossom-ci |
peterdsharpe
left a comment
There was a problem hiding this comment.
Submitting initial comments on everything except the notebooks (will review locally).
Other than the comments here, is it possible to save the PNG files (specifically resampled_volume_errors.png and variations_due_to_checkpoint.png) in a smaller file size? Right now those are adding ~25 MB to the repo size in total. Maybe JPEG or lower-res PNG?
Thanks! Compressed the images. |
|
/blossom-ci |
peterdsharpe
left a comment
There was a problem hiding this comment.
Overall looks good. Leaving my notebook comments here, since GitHub won't let me add line-by-line annotations within *.ipynb's.
General items:
- Please update changelog (warp version bump), Dependencies section.
A blocker in our notebooks, relating to us pointing users to deprecated APIs:
- The cell that runs NIM inference uses
_create_nbrs_surfaceand_interpolate. These are explicitly deprecated in the same file, and we direct users to useinterpolate_mesh_to_pcorphysicsnemo.nn.functional.neighbors.knninstead. Can we fix our tutorial code here to use non-deprecated APIs? The NIM produces a sparse point cloudoutput_dict["surface_coordinates"][0]with fields. Wrap it aspv.PolyData(coords)withpoint_datapopulated, then call interpolate_mesh_to_pc withpc=mesh_cell_centers, mesh=nim_pc, mesh_dtype="point", device="gpu". About 5 lines of glue.
Another blocker in our notebooks, related to credentials leaking:
The cells that run !wget "{url}" for drivaer_418.stl and drivaer_420.stl capture the redirect chain, including:
https://cas-bridge.xethub.hf.co/xet-bridge-us/.../drivaer_418.stl?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=cas%2F...&X-Amz-Signature=155cd1bfd12bedc99e9a5c8eafca8602bb1edbbf108495138d9d0af5906e971d&...
These are pre-signed S3-style URLs. They expired ~1 hour after generation (so this PR's URLs are dead now), but committing them is bad hygiene: it normalizes embedding presigned URLs in public artifacts, and future re-runs will commit live ones. Concrete fixes, in order of preference:
- Replace
!wget "{url}" -O "{filename}"withhuggingface_hub.hf_hub_download(already a transitive dep viaevaluation-hf). It logs nothing comparable. - Use
requests.get(url, stream=True)withiter_contentand a small progress bar that doesn't echo the URL. - At minimum, add
--quiettowgetand clear the output cells before commit.
Thanks, I have added |
|
/blossom-ci |
peterdsharpe
left a comment
There was a problem hiding this comment.
Approving after offline discussion.
One remaining item that we agreed to do before merge - re-running the notebooks to do a) end-to-end integration testing, and b) cleanup outdated output cells that could mislead end-users using this as primary documentation.
PhysicsNeMo-CFD Pull Request
Description
Tested the nim inference workflow and made minor edits as a part of the testing. The l2_error_vs_sdf now clip first and then compute the metrics. Also, device is handled better.
Checklist
Dependencies