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

Fix ngen entrypoint script for geopackage hydrofabrics #624

Merged

Conversation

robertbartel
Copy link
Contributor

Fixing to properly support geopackage hydrofabric dataset type in commands to execute ngen if that is what is provided.

Also adding creation of a symlink to output dataset in fixed location for routing.

Fixing to properly support geopackage hydrofabric dataset type in
commands to execute ngen if that is what is provided; also adding
creation of a symlink to output dataset in fixed location for routing.
@robertbartel robertbartel added bug Something isn't working maas MaaS Workstream labels May 17, 2024
Copy link
Contributor

@christophertubbs christophertubbs left a comment

Choose a reason for hiding this comment

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

The symbolic link for routing is questionable in my eyes. If it's important to rubberstamp a path named experiment_output and not something that stands to be a little more longer lasting, cool, I just want to make sure it's acknowledged.

@@ -48,6 +48,10 @@ init_ngen_executable_paths

# Move to the output dataset mounted directory
cd ${OUTPUT_DATASET_DIR:?Output dataset directory not defined}
#Needed for routing
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this hard coding intentional? I don't see any other references to /dmod/dataset/experiment_output and we should probably stay away from just naming stuff some form of experiment - we're still stuck with that in the GUI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hard coding is intentional. In short, we need to configure something in advance that will line up with something not known until runtime.

A troute config file needs to include the path to the ngen output (and its own output). But job worker have datasets mounted in as a directory with the same name as the dataset itself. Output dataset names are based on the job id, and that is dynamically generated (at least usually).

I realized after setting this up that we don't absolutely have to do this now, but I decided to leave it in. I could take it out, but it's a problem we eventually will have to deal with, in which case, why not at least do this for now?

I also don't object to changing the name of the symlink, if you have a strong preference about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI @christophertubbs, I opened #627 for this. Let me know if you think this needs to come out, or can be left in as a temporary fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, there's a ticket, so I'm good.


#Capture the return value to use as service exit code
NGEN_RETURN=$?
if [ -e ${HYDROFABRIC_DATASET_DIR}/catchment_data.geojson ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

These are probably more evidence that we need to bump the priority for the ticket for moving logic into python. It might just be because I generally avoid too much shell scripting, but I find this hard to read/track. It doesn't break convention with the rest of the script, so moving that out to another language isn't in the scope of this task.

@christophertubbs christophertubbs merged commit 8c7b9d1 into NOAA-OWP:master May 20, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working maas MaaS Workstream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants