Repository navigation
[fournos-launcher] Submit jobs to the psap-mgmt cluster - #19
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Kubernetes name-sanitization utilities and uses them for job name generation; adjusts shell logging literals; updates fournOS orchestration owner string and FournosJob template apiVersion and preset/configOverrides rendering; and relocates CI Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/core/dsl/shell.py`:
- Line 85: The two log lines use string placeholders but are missing the
f-string prefix, so update the logger.info calls that reference
result.stderr.strip() and result.returncode to use f-strings (add the leading
"f") so the placeholders are interpolated; locate the logger.info("| <stderr>
{result.stderr.strip()}") and logger.info("| Exit code: {result.returncode}")
occurrences in the shell execution routine and add the f prefix to each log
string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e57e3357-53ee-407b-8957-7b23a768da5b
📒 Files selected for processing (3)
projects/core/dsl/shell.pyprojects/fournos_launcher/orchestration/config.yamlprojects/fournos_launcher/toolbox/submit_and_wait/templates/job.yaml.j2
|
/test fournos llm_d hello world |
|
• Link to the test results. • No reports index generated... Test configuration: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/core/dsl/utils/k8s.py`:
- Around line 46-53: The current truncation and fallback for the sanitized name
can create duplicate names; change the logic around the sanitized variable so
truncation appends a deterministic short hash instead of blind slicing and the
empty-fallback uses a hashed suffix: compute a stable hash (e.g.,
sha1/hexdigest) of the original input, then when len(sanitized) > 63 produce a
name like sanitized[:(63-9)] + '-' + hash[:8] (ensure resulting last char is
alphanumeric and replace any non-alnum with 'x'), and when sanitized is empty
return 'default-' + hash[:8] (also ensure length<=63 and alphanumeric end).
Update the block that currently does if len(sanitized) > 63: ... and the return
sanitized or 'default' to implement this hashed-suffix strategy using the
variable sanitized and the original input used to build it.
In `@projects/fournos_launcher/toolbox/submit_and_wait/templates/job.yaml.j2`:
- Around line 13-14: The template currently injects preset with {{ args.args }}
which renders a Python list as a YAML string; change the expression to serialize
the list as JSON (same approach used for configOverrides) by using the tojson
filter on args.args so preset becomes a proper YAML/JSON array; update the
job.yaml.j2 template's preset line to use args.args | tojson (referencing the
preset key and args.args symbol and matching the existing configOverrides
usage).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7722bf52-2939-4956-886b-b7cfad31cbcd
📒 Files selected for processing (6)
projects/core/dsl/shell.pyprojects/core/dsl/utils/__init__.pyprojects/core/dsl/utils/k8s.pyprojects/fournos_launcher/orchestration/config.yamlprojects/fournos_launcher/toolbox/submit_and_wait/main.pyprojects/fournos_launcher/toolbox/submit_and_wait/templates/job.yaml.j2
✅ Files skipped from review due to trivial changes (3)
- projects/core/dsl/utils/init.py
- projects/core/dsl/shell.py
- projects/fournos_launcher/orchestration/config.yaml
| if len(sanitized) > 63: | ||
| sanitized = sanitized[:63] | ||
| # Make sure it still ends with alphanumeric after truncation | ||
| if not sanitized[-1].isalnum(): | ||
| sanitized = sanitized[:-1] + 'x' | ||
|
|
||
| return sanitized or 'default' | ||
|
|
There was a problem hiding this comment.
Avoid collision-prone truncation/fallback in sanitized names.
Current logic at Line 46 and Line 52 can produce duplicate names ([:63] truncation or 'default' fallback), which may cause job creation conflicts.
Suggested fix
+import hashlib
import re
@@
- # Truncate to 63 characters (K8s limit)
- if len(sanitized) > 63:
- sanitized = sanitized[:63]
- # Make sure it still ends with alphanumeric after truncation
- if not sanitized[-1].isalnum():
- sanitized = sanitized[:-1] + 'x'
-
- return sanitized or 'default'
+ # Keep uniqueness when truncating or recovering from empty names
+ if len(sanitized) > 63 or not sanitized:
+ suffix = hashlib.sha1(name.encode("utf-8")).hexdigest()[:8]
+ head = (sanitized or "job")[:63 - len(suffix) - 1]
+ head = re.sub(r'-+$', '', head) or "job"
+ sanitized = f"{head}-{suffix}"
+
+ if not sanitized[-1].isalnum():
+ sanitized = sanitized[:-1] + 'x'
+
+ return sanitized📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(sanitized) > 63: | |
| sanitized = sanitized[:63] | |
| # Make sure it still ends with alphanumeric after truncation | |
| if not sanitized[-1].isalnum(): | |
| sanitized = sanitized[:-1] + 'x' | |
| return sanitized or 'default' | |
| # Keep uniqueness when truncating or recovering from empty names | |
| if len(sanitized) > 63 or not sanitized: | |
| suffix = hashlib.sha1(name.encode("utf-8")).hexdigest()[:8] | |
| head = (sanitized or "job")[:63 - len(suffix) - 1] | |
| head = re.sub(r'-+$', '', head) or "job" | |
| sanitized = f"{head}-{suffix}" | |
| if not sanitized[-1].isalnum(): | |
| sanitized = sanitized[:-1] + 'x' | |
| return sanitized |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/core/dsl/utils/k8s.py` around lines 46 - 53, The current truncation
and fallback for the sanitized name can create duplicate names; change the logic
around the sanitized variable so truncation appends a deterministic short hash
instead of blind slicing and the empty-fallback uses a hashed suffix: compute a
stable hash (e.g., sha1/hexdigest) of the original input, then when
len(sanitized) > 63 produce a name like sanitized[:(63-9)] + '-' + hash[:8]
(ensure resulting last char is alphanumeric and replace any non-alnum with 'x'),
and when sanitized is empty return 'default-' + hash[:8] (also ensure length<=63
and alphanumeric end). Update the block that currently does if len(sanitized) >
63: ... and the return sanitized or 'default' to implement this hashed-suffix
strategy using the variable sanitized and the original input used to build it.
| preset: {{ args.args }} | ||
| configOverrides: {{ args.variables_overrides | tojson }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the job.yaml.j2 file
cat -n "projects/fournos_launcher/toolbox/submit_and_wait/templates/job.yaml.j2"Repository: openshift-psap/forge
Length of output: 626
🏁 Script executed:
# Search for references to this template and how args.args is constructed
rg "job\.yaml\.j2|args\.args" --type py --type yaml -B 2 -A 2 | head -100Repository: openshift-psap/forge
Length of output: 2279
🏁 Script executed:
# Find related Python files that might populate args
fd "submit_and_wait" --type f | head -20Repository: openshift-psap/forge
Length of output: 46
🏁 Script executed:
# Search for preset usage and any schema definitions
rg "preset" --type py --type yaml -B 2 -A 2 | head -80Repository: openshift-psap/forge
Length of output: 5185
🏁 Script executed:
# Look for test files related to submit_and_wait
find . -path "*/test*" -name "*submit_and_wait*" -o -path "*/test*" -name "*job.yaml*" | head -20Repository: openshift-psap/forge
Length of output: 46
🏁 Script executed:
# Let's test what Jinja2 renders for a Python list
python3 <<'EOF'
from jinja2 import Template
# Test default rendering of Python list
template = Template("{{ my_list }}")
result = template.render(my_list=['item1', 'item2', 'special"quote'])
print("Default rendering:", repr(result))
# Test with tojson filter
template_json = Template("{{ my_list | tojson }}")
result_json = template_json.render(my_list=['item1', 'item2', 'special"quote'])
print("With tojson filter:", repr(result_json))
# What this looks like in YAML context
yaml_test_default = f"""
preset: {result}
"""
print("\nYAML with default rendering:")
print(yaml_test_default)
yaml_test_json = f"""
preset: {result_json}
"""
print("YAML with tojson rendering:")
print(yaml_test_json)
EOFRepository: openshift-psap/forge
Length of output: 337
🏁 Script executed:
# Search for Fournos schema or documentation
rg "FournosJob|preset" --type yaml --type python | grep -i "schema\|preset\|fournos" | head -40Repository: openshift-psap/forge
Length of output: 94
🏁 Script executed:
# Look for any test files or example manifests
find . -name "*.yaml" -o -name "*.yml" | xargs grep -l "preset" 2>/dev/null | head -10Repository: openshift-psap/forge
Length of output: 145
🏁 Script executed:
# Check for more context on how args.args is populated
rg "args\.args\s*=" --type py -B 3 -A 1 | head -60Repository: openshift-psap/forge
Length of output: 46
Serialize preset safely before injecting into YAML.
At Line 13, {{ args.args }} renders a Python list as a string literal (e.g., ['item1', 'item2']), which YAML parses as a string rather than an array, causing a type mismatch. Use tojson to produce proper JSON syntax, matching the approach already used for configOverrides at Line 14.
Suggested fix
- preset: {{ args.args }}
+ preset: {{ args.args | tojson }}
configOverrides: {{ args.variables_overrides | tojson }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| preset: {{ args.args }} | |
| configOverrides: {{ args.variables_overrides | tojson }} | |
| preset: {{ args.args | tojson }} | |
| configOverrides: {{ args.variables_overrides | tojson }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/fournos_launcher/toolbox/submit_and_wait/templates/job.yaml.j2`
around lines 13 - 14, The template currently injects preset with {{ args.args }}
which renders a Python list as a YAML string; change the expression to serialize
the list as JSON (same approach used for configOverrides) by using the tojson
filter on args.args so preset becomes a proper YAML/JSON array; update the
job.yaml.j2 template's preset line to use args.args | tojson (referencing the
preset key and args.args symbol and matching the existing configOverrides
usage).
|
/test fournos llm_d hello world |
|
• Link to the test results. • No reports index generated... Test configuration: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/core/dsl/toolbox.py`:
- Around line 115-118: The CalledProcessError handling in the generator assumes
e.original_exception.args[0] is a message string; change the condition in the
branch that checks isinstance(e.original_exception,
subprocess.CalledProcessError) to also verify that e.original_exception.args is
truthy and that e.original_exception.args[0] is a non-empty string (e.g.,
isinstance(...args[0], str) and args[0].strip()). If that check passes, yield
the args[0] message as before; otherwise fall back to yielding a stringified
version of the exception (e.g., str(e.original_exception)) so integer return
codes do not render as the message. Ensure you reference e.original_exception
and subprocess.CalledProcessError in the updated condition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29031f3b-c8c3-4d5d-95ff-5ee37a043f4a
📒 Files selected for processing (2)
projects/core/dsl/toolbox.pyprojects/fournos_launcher/toolbox/submit_and_wait/main.py
🚧 Files skipped from review as they are similar to previous changes (1)
- projects/fournos_launcher/toolbox/submit_and_wait/main.py
| if isinstance(e.original_exception, subprocess.CalledProcessError) and e.original_exception.args: | ||
| yield f"~~ {e.original_exception.args[0]}" | ||
| else: | ||
| yield f"~~ {e.original_exception}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all construction paths for CalledProcessError and where .args is overridden.
# Expected result: any path without string args[0] can trigger low-quality error rendering.
rg -nP --type=py -C3 '\bCalledProcessError\s*\('
rg -nP --type=py -C3 '\.args\s*='Repository: openshift-psap/forge
Length of output: 1455
🏁 Script executed:
#!/bin/bash
# 1. Search for all CalledProcessError instantiations
echo "=== CalledProcessError instantiations ==="
rg -nP --type=py 'CalledProcessError\s*\(' -A2
echo ""
echo "=== All .args assignments ==="
rg -nP --type=py '\.args\s*=' -B2 -A2
echo ""
echo "=== Context around toolbox.py lines 114-120 to see the full error handling ==="
sed -n '110,125p' projects/core/dsl/toolbox.pyRepository: openshift-psap/forge
Length of output: 1629
🏁 Script executed:
#!/bin/bash
python3 <<'EOF'
import subprocess
# Simulate what happens in runner.py:67
try:
raise subprocess.CalledProcessError(1, "test_command", "stdout_output", "stderr_output")
except subprocess.CalledProcessError as e:
print("CalledProcessError created directly (runner.py style):")
print(f" .args = {e.args}")
print(f" .args[0] = {e.args[0]}")
print(f" type(args[0]) = {type(e.args[0])}")
print()
# Simulate what happens in shell.py:106-107
try:
error = subprocess.CalledProcessError(1, "test_command", "stdout_output", "stderr_output")
error.args = ("Enhanced error message here",)
raise error
except subprocess.CalledProcessError as e:
print("CalledProcessError with .args override (shell.py style):")
print(f" .args = {e.args}")
print(f" .args[0] = {e.args[0]}")
print(f" type(args[0]) = {type(e.args[0])}")
EOFRepository: openshift-psap/forge
Length of output: 403
🏁 Script executed:
#!/bin/bash
# View the shell.py error message construction
sed -n '95,110p' projects/core/dsl/shell.pyRepository: openshift-psap/forge
Length of output: 720
Harden CalledProcessError message extraction to prevent rendering integer return codes.
The current code assumes args[0] is always a message string, but when CalledProcessError is raised directly (as in runner.py), args[0] is the integer return code. This produces output like ~~ 1 instead of an error message. The check should verify args[0] is both a string and non-empty before using it.
Proposed patch
- if isinstance(e.original_exception, subprocess.CalledProcessError) and e.original_exception.args:
- yield f"~~ {e.original_exception.args[0]}"
+ if (
+ isinstance(e.original_exception, subprocess.CalledProcessError)
+ and e.original_exception.args
+ and isinstance(e.original_exception.args[0], str)
+ and e.original_exception.args[0].strip()
+ ):
+ yield f"~~ {e.original_exception.args[0]}"
else:
yield f"~~ {e.original_exception}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isinstance(e.original_exception, subprocess.CalledProcessError) and e.original_exception.args: | |
| yield f"~~ {e.original_exception.args[0]}" | |
| else: | |
| yield f"~~ {e.original_exception}" | |
| if ( | |
| isinstance(e.original_exception, subprocess.CalledProcessError) | |
| and e.original_exception.args | |
| and isinstance(e.original_exception.args[0], str) | |
| and e.original_exception.args[0].strip() | |
| ): | |
| yield f"~~ {e.original_exception.args[0]}" | |
| else: | |
| yield f"~~ {e.original_exception}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@projects/core/dsl/toolbox.py` around lines 115 - 118, The CalledProcessError
handling in the generator assumes e.original_exception.args[0] is a message
string; change the condition in the branch that checks
isinstance(e.original_exception, subprocess.CalledProcessError) to also verify
that e.original_exception.args is truthy and that e.original_exception.args[0]
is a non-empty string (e.g., isinstance(...args[0], str) and args[0].strip()).
If that check passes, yield the args[0] message as before; otherwise fall back
to yielding a stringified version of the exception (e.g.,
str(e.original_exception)) so integer return codes do not render as the message.
Ensure you reference e.original_exception and subprocess.CalledProcessError in
the updated condition.
|
/test fournos llm_d hello world |
|
• Link to the test results. • No reports index generated... Test configuration: |
4ee1180 to
05d0825
Compare
|
/test fournos llm_d hello world |
|
• Link to the test results. • No reports index generated... Test configuration: |
|
/test jump-ci llm_d cks |
1 similar comment
|
/test jump-ci llm_d cks |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/fournos_launcher/toolbox/submit_and_wait/main.py`:
- Around line 104-107: The auto-generated name can lose the timestamp when
sanitize_k8s_name truncates a long raw_name; update the logic that builds
raw_name so it truncates args.project to reserve space for the "-{timestamp}"
suffix (and the "forge-" prefix) before calling sanitize_k8s_name, i.e., compute
a maxProjectLen = MAX_K8S_NAME_LEN - len("forge-") - len("-") - len(timestamp)
and use args.project[:maxProjectLen] when composing raw_name so
ctx.final_job_name = sanitize_k8s_name(raw_name) always preserves the timestamp.
In `@projects/jump_ci/toolbox/jump_ci_prepare_step/tasks/main.yml`:
- Around line 58-65: Create the missing directory before writing
variable_overrides.yaml: add an Ansible task that ensures the directory "{{
step_artifact_dir }}/artifacts/000__ci_metadata" exists (use
ansible.builtin.file or file with state: directory and proper mode/owner) and
place it before the here-doc that writes to "{{ step_artifact_dir
}}/artifacts/000__ci_metadata/variable_overrides.yaml"; this guarantees the path
for the subsequent ansible.posix.synchronize task copying to "{{
artifact_extra_logs_dir }}/000__ci_metadata/variable_overrides.yaml" will
succeed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d4955f4-47a1-4a9f-84f2-5a4c76eae290
📒 Files selected for processing (9)
projects/core/dsl/toolbox.pyprojects/core/dsl/utils/__init__.pyprojects/core/dsl/utils/k8s.pyprojects/fournos_launcher/orchestration/config.yamlprojects/fournos_launcher/toolbox/submit_and_wait/main.pyprojects/fournos_launcher/toolbox/submit_and_wait/templates/job.yaml.j2projects/jump_ci/testing/test.pyprojects/jump_ci/toolbox/jump_ci_prepare_step/tasks/main.ymlprojects/legacy/library/config.py
✅ Files skipped from review due to trivial changes (1)
- projects/core/dsl/utils/init.py
🚧 Files skipped from review as they are similar to previous changes (4)
- projects/core/dsl/toolbox.py
- projects/fournos_launcher/toolbox/submit_and_wait/templates/job.yaml.j2
- projects/fournos_launcher/orchestration/config.yaml
- projects/core/dsl/utils/k8s.py
|
/test jump-ci llm_d cks |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@projects/jump_ci/toolbox/jump_ci_prepare_step/tasks/main.yml`:
- Around line 65-66: The synchronize task copies to "{{ artifact_extra_logs_dir
}}/000__ci_metadata/variable_overrides.yaml" but the parent directory
"000__ci_metadata" may not exist; add an ansible.builtin.file task before the
synchronize step to ensure the destination directory exists (create "{{
artifact_extra_logs_dir }}/000__ci_metadata" with state: directory,
recurse/owner/mode as appropriate) so the synchronize targeting
variable_overrides.yaml succeeds; reference the variables
artifact_extra_logs_dir and step_artifact_dir to locate where to insert the new
file task.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 578045c5-261b-44e5-a0a6-36531437b869
📒 Files selected for processing (3)
projects/jump_ci/testing/test.pyprojects/jump_ci/toolbox/jump_ci_prepare_step/tasks/main.ymlprojects/legacy/library/config.py
✅ Files skipped from review due to trivial changes (1)
- projects/legacy/library/config.py
|
/test jump-ci llm_d cks |
|
/test jump-ci llm_d cks |
|
/test jump-ci llm_d cks |
|
/test jump-ci llm_d cks |
|
• Link to the test results. • No reports index generated... Test configuration: |
|
/test fournos llm_d cks |
|
• Link to the test results. • No reports index generated... Test configuration: |
|
ok, good enough, I merge this 👍🏻 |
Summary by CodeRabbit
New Features
Bug Fixes
Chores