-
Notifications
You must be signed in to change notification settings - Fork 7
Register rdas client in jcb. Bug fix for running on HPC (hera). #30
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
Conversation
jcb_client_init.py
Outdated
| #write_message(f'Cloning {app} with command: {command_string}') | ||
| print(f'Cloning {app} with command: {command_string}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason why this had to be changed? write_message is used in other places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would hang with write_message (on Hera). Switching to print would allow it to continue. I don't fully understand what the problem was. I can look into it more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't spend too much time on it but if it's a python version issue or something that we can easily explain that would be helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I figured it out. I didn't notice there was a max_line_length. My path to where jcb_client_init.py is cloning the jcb clients is too long and the logic would end up doing an infinite loop. I could double max_line_length (to 200) and/or add some logic to force split the lines up in the case where 200 wouldn't be enough and would again cause an infinite loop (safer). What would you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter if it is not too difficult, the former if it needs to be something quick and easy
…rd/path may be longer than max_line_length.
|
@CoryMartin-NOAA any idea why unit test 3.13 is failing? Is it because I'm trying to merge a branch from my fork? |
|
@delippi yeah that's my guess. I wouldn't worry, I can bypass the CI and can hotfix if necessary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR registers the RDAS client in JCB and fixes a bug in the message formatting function that could cause infinite loops when handling long words without spaces.
- Adds RDAS client configuration to enable RDAS workflow support in JCB
- Fixes infinite loop bug in
write_messagefunction when processing words longer than 100 characters
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| jcb_clients.yaml | Adds RDAS client registration with repository and branch configuration |
| jcb_client_init.py | Fixes infinite loop bug in message line-breaking logic for long words |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@delippi take a look at the actions, it looks like the failure was real after all, I think there's an expected template missing for RDAS? I didn't look closely |
|
@CoryMartin-NOAA, it is failing at a different step than it was earlier. I don't think it ran this step before because it was first failing to checkout the code. I think it is because |
|
@delippi for application specific code, like if you had an OOPS application that did some sort of LBC/halo processing that only applied to the regional system, the template would live here |
|
@CoryMartin-NOAA how do I manually run the ctests on hera to debug this? I think I have a simple fix to at least not have the ctests failing. Still lots of work to do to get this running for rdas. |
|
@delippi I think you install jcb into your virtual environment and then run jcb/.github/workflows/basic_testing.yaml Lines 68 to 79 in ebd10ea
|
This PR registers the RDAS client in JCB, enabling support for RDAS workflows. In addition, it includes a bug fix needed to ensure successful runs on Hera. These updates are part of the ongoing effort to move RDAS under JCB.