Rename accelerator_flavor to keep both local and global variables #1783
+4
−4
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -346,8 +346,8 @@ function _handle_test() | |
| local notebook_id= | ||
|
|
||
| # Due to existing logic - cuda accelerator value needs to be treated as empty string | ||
| local accelerator="${accelerator_flavor}" | ||
| accelerator="${accelerator_flavor##'cuda'}" | ||
|
Comment on lines
+349
to
+350
There was a problem hiding this comment. 💡 Verification agent 🧩 Analysis chainGood fix; consider collapsing to a single assignment for clarity Keeping Apply this minimal diff: - local accelerator="${accelerator_flavor}"
- accelerator="${accelerator_flavor##'cuda'}"
+ local accelerator="${accelerator_flavor##'cuda'}"Optionally verify no other unintended mutations of 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Look for places removing prefixes from accelerator_flavor (potentially problematic if global)
rg -n -- '\$\{accelerator_flavor##' || true
# Look for any reassignments of accelerator_flavor that depend on its own value
rg -n -- '^\s*accelerator_flavor=.*\$\{accelerator_flavor' || true
# Look for prefix-insertion sites still using accelerator_flavor (should be local accelerator in _handle_test())
rg -n -- '\$\{accelerator_flavor:\+\$accelerator_flavor/' || trueLength of output: 283 Collapse redundant The initial local accelerator="${accelerator_flavor}"is immediately overridden by accelerator="${accelerator_flavor##'cuda'}"so you can simplify to a single assignment without changing behavior. Files to update:
Proposed diff: - local accelerator="${accelerator_flavor}"
- accelerator="${accelerator_flavor##'cuda'}"
+ local accelerator="${accelerator_flavor##'cuda'}"🤖 Prompt for AI AgentsThere was a problem hiding this comment. shellcheck dislikes I would not mess with this There was a problem hiding this comment. @jiridanek, you're absolutely right. ShellCheck would indeed flag The current two-line approach in the PR is the correct pattern for maximum portability: local accelerator="${accelerator_flavor}"
accelerator="${accelerator_flavor##'cuda'}"I withdraw my suggestion to collapse these lines. The fix as implemented properly addresses the original issue while maintaining shell script best practices. Thanks for catching that! 🧠 Learnings used |
||
|
|
||
|
|
||
| case "${notebook_workload_name}" in | ||
|
|
@@ -361,10 +361,10 @@ function _handle_test() | |
| notebook_id="${jupyter_trustyai_notebook_id}" | ||
| ;; | ||
| *${jupyter_tensorflow_notebook_id}-*) | ||
| notebook_id="${accelerator:+$accelerator/}${jupyter_tensorflow_notebook_id}" | ||
| ;; | ||
| *${jupyter_pytorch_notebook_id}-*) | ||
| notebook_id="${accelerator:+$accelerator/}${jupyter_pytorch_notebook_id}" | ||
| ;; | ||
| *) | ||
| printf '%s\n' "No matching condition found for ${notebook_workload_name}." | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@coderabbitai explain to me what just happened there, I don't understand the Bash magic of
accelerator="${accelerator_flavor##'cuda'}", and${accelerator:+$accelerator/},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.
@jiridanek
Short version:
accelerator="${accelerator_flavor##'cuda'}"
strips the longest match of the pattern from the start of the string.
"${accelerator:+$accelerator/}"
Why: CUDA should have no prefix in paths (default), ROCm should use "rocm/". We compute a local accelerator for path-building without clobbering the global accelerator_flavor (which is still needed later for imagestream selection). If you prefer less “Bash magic,” this is equivalent and a bit more explicit:
If you want, I can rewrite it to this more explicit form for readability.
🧠 Learnings used
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.
no need, I see that the tests are now progressing further, looking at https://github.com/daniellutz/odh-notebooks/actions/runs/16835369495/job/47693428021, so this does fix problem
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.
/lgtm
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.
@jiridanek Thanks! Glad this unblocked the tests. Nothing else from my side.
🧠 Learnings used
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
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.
@jiridanek Great—thanks for confirming. Glad the tests are moving and the fix resolves the issue. No further changes from my side; happy to convert to the explicit case-form later if you ever want it for readability.
✏️ Learnings added
🧠 Learnings used
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!