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

ALCF fixes: tiff_to_zarr.py dynamic pixelsize, init_tiff_to_zarr_globus_flow.py, fix for issue #27 #29

Merged
merged 4 commits into from
Oct 7, 2024

Conversation

davramov
Copy link
Contributor

@davramov davramov commented Oct 2, 2024

  • Updated tiff_to_zarr.py to read pixelsize from the raw h5 file and convert to micrometer for the multiscales step.
  • Updated init_tiff_to_zarr_globus_flow.py to account for the new change (passing in the h5 path to the flow) as well as automatically updating the Prefect Secret Block on the server with the latest flow_id and func_id once the script runs. Side note: I removed the time import from the function definition because I was getting a cryptic "time.run() is not defined error" despite only calling time.time().
  • Updated instructions for running check_globus_compute.py in the docstring.

…nvert to micrometer for the multiscales step. Updated init_tiff_to_zarr_globus_flow.py to account for the new change (passing in the h5 path to the flow) as well as automatically updating the Prefect Secret Block on the server with the new flow_id and func_id. Updated instructions for running check_globus_compute.py in the docstring.
@davramov davramov changed the title tiff_to_zarr.py dynamic pixelsize ALCF fixes: tiff_to_zarr.py dynamic pixelsize, init_tiff_to_zarr_globus_flow.py, fix for issue #27 Oct 2, 2024
@@ -30,6 +37,13 @@ def set_permissions_recursive(path, permissions=0o2775):
os.chmod(path, permissions) # Also set permissions for the top-level directory


def read_pixelsize_from_hdf5(raw_directory: str) -> dict:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is picky, but shouldn't the parameter be named raw_file, not raw_directory?

scripts/polaris/tiff_to_zarr.py Show resolved Hide resolved
@@ -42,14 +39,13 @@ def conversion_wrapper(rundir, h5_file_name, folder_path):
file_name = h5_file_name[:-3] if h5_file_name.endswith('.h5') else h5_file_name
command = (
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove hard-coded paths?

@@ -76,6 +72,17 @@ def create_flow_definition():
return flow_definition


@task(name="update-flow-in-Prefect")
Copy link
Contributor

Choose a reason for hiding this comment

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

These look to me like they should be json blocks and not Secrets, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, let's stick to snake_case for function names. I know, picky.

@@ -31,9 +31,6 @@ def conversion_wrapper(rundir, h5_file_name, folder_path):
"""
import os
import subprocess
import time
Copy link
Contributor

@dylanmcreynolds dylanmcreynolds Oct 2, 2024

Choose a reason for hiding this comment

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

Was the issue that references time.run() at the import or the use of time.time()

That's super duper odd. I would be interested to see print(time.time) if it's not occuring during the import.

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 couldn't recreate the issue locally but only when the Globus Flow called the function. There error listed specifically the line start = time.time(). What's odd to me is A) it was working before and B) I am calling time.time() and nowhere do I call time.run().

…ration/_tests/test_globus_flow.py to include a test for process_new_832_file_flow() in orchestration/flows/bl832/move.py (note that this still needs work to properly validate the data pruning task). Updated move.py to accept a config as input (similar to what we did in the ALCF script) so we can inject Config832 from the test script. Fixed a typo in orchestration/flows/bl832/prune.py where there was missing parantheses causing an issue on the flow-prd agent. Updated scripts/init_tiff_to_zarr_globus_flow.py, tiff_to_zarr.py, and alcf.py to address hardcoded path concerns.
@dylanmcreynolds
Copy link
Contributor

Fixes #27

@dylanmcreynolds dylanmcreynolds merged commit ae13984 into als-computing:main Oct 7, 2024
1 check passed
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