fix: remap container paths to host paths in job_metadata.json#218
Merged
k-chrispens merged 1 commit intomainfrom Apr 14, 2026
Merged
fix: remap container paths to host paths in job_metadata.json#218k-chrispens merged 1 commit intomainfrom
k-chrispens merged 1 commit intomainfrom
Conversation
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 50 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdded host-path remapping for guidance job metadata: documented usage in the docker entrypoint, passed host-path env vars into container runs, implemented container→host path remapping in GuidanceConfig.as_dict(), and extended tests to cover remapping behavior and precedence. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/utils/test_guidance_script_arguments.py (1)
247-320: Drop_resolve_checkpointmocks fromas_dictremap tests.These tests don’t exercise checkpoint resolution, so the patching adds avoidable coupling.
As per coding guidelines " `**/test_*.py`: Write black-box tests that verify behavior, not implementation. Use realistic inputs and avoid mocks. Test public interfaces with expected behaviors."🧪 Simplify tests by removing unnecessary mocks
-@patch( - "sampleworks.utils.guidance_script_arguments._resolve_checkpoint", - return_value="/checkpoints/mock.ckpt", -) -def test_as_dict_remaps_all_four_path_fields(_mock_resolve, monkeypatch): +def test_as_dict_remaps_all_four_path_fields(monkeypatch): @@ -@patch( - "sampleworks.utils.guidance_script_arguments._resolve_checkpoint", - return_value="/checkpoints/mock.ckpt", -) -def test_as_dict_unchanged_when_no_env_vars(_mock_resolve, monkeypatch): +def test_as_dict_unchanged_when_no_env_vars(monkeypatch): @@ -@patch( - "sampleworks.utils.guidance_script_arguments._resolve_checkpoint", - return_value="/checkpoints/mock.ckpt", -) -def test_as_dict_remaps_with_catchall_host_dir(_mock_resolve, monkeypatch): +def test_as_dict_remaps_with_catchall_host_dir(monkeypatch):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils/test_guidance_script_arguments.py` around lines 247 - 320, The tests test_as_dict_remaps_all_four_path_fields, test_as_dict_unchanged_when_no_env_vars, and test_as_dict_remaps_with_catchall_host_dir are unnecessarily patching sampleworks.utils.guidance_script_arguments._resolve_checkpoint; remove the `@patch`(...) decorator from each test and delete the unused _mock_resolve parameter from their function signatures so the tests exercise GuidanceConfig.as_dict as a black-box without coupling to internal checkpoint resolution logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sampleworks/utils/guidance_script_arguments.py`:
- Around line 84-85: The code appends a normalized host path with
host_dir.rstrip("/") which turns "/" into "" and also doesn't remove surrounding
whitespace before use; change the normalization so you first trim whitespace
from host_dir (host_dir = host_dir.strip()), then compute normalized_host = "/"
if host_dir == "/" else host_dir.rstrip("/"), and use that in the
env_pairs.append call (replace the existing
env_pairs.append((container_prefix.rstrip("/"), host_dir.rstrip("/")))). Ensure
you still validate host_dir is non-empty after stripping and keep
container_prefix normalization as before.
---
Nitpick comments:
In `@tests/utils/test_guidance_script_arguments.py`:
- Around line 247-320: The tests test_as_dict_remaps_all_four_path_fields,
test_as_dict_unchanged_when_no_env_vars, and
test_as_dict_remaps_with_catchall_host_dir are unnecessarily patching
sampleworks.utils.guidance_script_arguments._resolve_checkpoint; remove the
`@patch`(...) decorator from each test and delete the unused _mock_resolve
parameter from their function signatures so the tests exercise
GuidanceConfig.as_dict as a black-box without coupling to internal checkpoint
resolution logic.
🪄 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: af2d41c9-1668-4891-9689-4d0a5dd7f802
📒 Files selected for processing (4)
docker-entrypoint.shrun_all_models.shsrc/sampleworks/utils/guidance_script_arguments.pytests/utils/test_guidance_script_arguments.py
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/utils/test_guidance_script_arguments.py (2)
202-208: ClearSAMPLEWORKS_HOST_RESULTS_DIRhere to avoid env-dependent flakiness.This test asserts catch-all behavior for
/data/results, but it never unsetsSAMPLEWORKS_HOST_RESULTS_DIR. If that var exists in the parent environment, this assertion can fail nondeterministically.Proposed fix
def test_remap_specific_env_takes_priority_over_catchall(monkeypatch): - + monkeypatch.delenv("SAMPLEWORKS_HOST_RESULTS_DIR", raising=False) monkeypatch.setenv("SAMPLEWORKS_HOST_INPUT_DIR", "/specific/data") monkeypatch.setenv("SAMPLEWORKS_HOST_DIR", "/general") assert _remap_container_path("/data/inputs/foo.cif") == "/specific/data/foo.cif"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils/test_guidance_script_arguments.py` around lines 202 - 208, The test can be flaky if SAMPLEWORKS_HOST_RESULTS_DIR exists in the environment; before asserting catch-all behavior for /data/results, remove that env var so _remap_container_path cannot pick a specific results mapping. In the test_remap_specific_env_takes_priority_over_catchall function use monkeypatch.delenv("SAMPLEWORKS_HOST_RESULTS_DIR", raising=False) (or equivalent) prior to the assertions so only SAMPLEWORKS_HOST_INPUT_DIR and SAMPLEWORKS_HOST_DIR control the outcome.
165-320: New test functions are missing docstrings required by repo rules.Most newly added tests in this block have no function docstrings. Please add concise NumPy-style docstrings to keep this file consistent with the project standard.
As per coding guidelines: "
**/*.py: Always include NumPy-style docstrings for every function and class."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils/test_guidance_script_arguments.py` around lines 165 - 320, Add concise NumPy-style docstrings to each new test function (e.g. test_remap_noop_when_no_env_vars, test_remap_data_dir_env, test_remap_results_dir_env, test_remap_catchall_dir_env, test_remap_specific_env_takes_priority_over_catchall, test_remap_leaves_checkpoint_unchanged, test_remap_trailing_slash_normalization, test_remap_exact_prefix_match, test_remap_ignores_empty_env_var, test_as_dict_remaps_all_four_path_fields, test_as_dict_unchanged_when_no_env_vars, test_as_dict_remaps_with_catchall_host_dir) by adding a one-line summary and a short description in NumPy docstring format directly under each def; keep them minimal (purpose of the test, key conditions/assertions) to satisfy the repo rule requiring NumPy-style docstrings for every function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/utils/test_guidance_script_arguments.py`:
- Around line 219-239: Add two regression tests in
tests/utils/test_guidance_script_arguments.py around the existing
_remap_container_path tests: (1) a whitespace-padded env var case that sets
SAMPLEWORKS_HOST_INPUT_DIR to " /mnt/data " and asserts
_remap_container_path("/data/inputs/foo.cif") returns "/mnt/data/foo.cif"
(ensuring trimming), and (2) a root env var case that sets
SAMPLEWORKS_HOST_INPUT_DIR to "/" and asserts
_remap_container_path("/data/inputs/foo.cif") returns "/foo.cif" and
_remap_container_path("/data/inputs") returns "/" (ensuring "/" normalizes
correctly); use monkeypatch to set/delenv like the other tests and reference
_remap_container_path to locate behavior to lock in.
---
Nitpick comments:
In `@tests/utils/test_guidance_script_arguments.py`:
- Around line 202-208: The test can be flaky if SAMPLEWORKS_HOST_RESULTS_DIR
exists in the environment; before asserting catch-all behavior for
/data/results, remove that env var so _remap_container_path cannot pick a
specific results mapping. In the
test_remap_specific_env_takes_priority_over_catchall function use
monkeypatch.delenv("SAMPLEWORKS_HOST_RESULTS_DIR", raising=False) (or
equivalent) prior to the assertions so only SAMPLEWORKS_HOST_INPUT_DIR and
SAMPLEWORKS_HOST_DIR control the outcome.
- Around line 165-320: Add concise NumPy-style docstrings to each new test
function (e.g. test_remap_noop_when_no_env_vars, test_remap_data_dir_env,
test_remap_results_dir_env, test_remap_catchall_dir_env,
test_remap_specific_env_takes_priority_over_catchall,
test_remap_leaves_checkpoint_unchanged, test_remap_trailing_slash_normalization,
test_remap_exact_prefix_match, test_remap_ignores_empty_env_var,
test_as_dict_remaps_all_four_path_fields,
test_as_dict_unchanged_when_no_env_vars,
test_as_dict_remaps_with_catchall_host_dir) by adding a one-line summary and a
short description in NumPy docstring format directly under each def; keep them
minimal (purpose of the test, key conditions/assertions) to satisfy the repo
rule requiring NumPy-style docstrings for every function.
🪄 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: 64f757db-610b-46d5-a628-f0ae41e0a331
📒 Files selected for processing (4)
docker-entrypoint.shrun_all_models.shsrc/sampleworks/utils/guidance_script_arguments.pytests/utils/test_guidance_script_arguments.py
✅ Files skipped from review due to trivial changes (2)
- docker-entrypoint.sh
- run_all_models.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sampleworks/utils/guidance_script_arguments.py
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/utils/test_guidance_script_arguments.py (1)
165-323: Add NumPy-style docstrings to newly added test functions.The new test functions are missing the required NumPy-style docstrings for functions/classes in Python files.
As per coding guidelines, "
**/*.py: Always include NumPy-style docstrings for every function and class."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils/test_guidance_script_arguments.py` around lines 165 - 323, Add NumPy-style docstrings to each newly added test function so they comply with the repository guideline requiring docstrings for every function/class; update the test functions test_remap_noop_when_no_env_vars, test_remap_data_dir_env, test_remap_results_dir_env, test_remap_catchall_dir_env, test_remap_specific_env_takes_priority_over_catchall, test_remap_leaves_checkpoint_unchanged, test_remap_trailing_slash_normalization, test_remap_exact_prefix_match, test_remap_ignores_empty_env_var, test_remap_whitespace_padded_env_var, test_remap_root_env_var, test_as_dict_remaps_all_four_path_fields, test_as_dict_unchanged_when_no_env_vars, and test_as_dict_remaps_with_catchall_host_dir to include concise NumPy-style docstrings (one-line summary plus short description/notes where appropriate) describing the test purpose and expected behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/utils/test_guidance_script_arguments.py`:
- Around line 12-13: Replace direct tests of the private helper
_remap_container_path by exercising the public GuidanceConfig.as_dict()
interface: remove imports/usages of _remap_container_path from tests and extend
the three existing as_dict() test functions to include the edge cases covered
previously (trailing slashes, leading/trailing whitespace, and root container
paths) by constructing GuidanceConfig instances with those host/container path
variants and asserting the expected remapped values appear in the as_dict()
output; locate references to _remap_container_path and the
GuidanceConfig.as_dict() calls to update assertions accordingly.
---
Nitpick comments:
In `@tests/utils/test_guidance_script_arguments.py`:
- Around line 165-323: Add NumPy-style docstrings to each newly added test
function so they comply with the repository guideline requiring docstrings for
every function/class; update the test functions
test_remap_noop_when_no_env_vars, test_remap_data_dir_env,
test_remap_results_dir_env, test_remap_catchall_dir_env,
test_remap_specific_env_takes_priority_over_catchall,
test_remap_leaves_checkpoint_unchanged, test_remap_trailing_slash_normalization,
test_remap_exact_prefix_match, test_remap_ignores_empty_env_var,
test_remap_whitespace_padded_env_var, test_remap_root_env_var,
test_as_dict_remaps_all_four_path_fields,
test_as_dict_unchanged_when_no_env_vars, and
test_as_dict_remaps_with_catchall_host_dir to include concise NumPy-style
docstrings (one-line summary plus short description/notes where appropriate)
describing the test purpose and expected behavior.
🪄 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: 56da6970-0858-46a2-bf2e-c2f45b3ef3fb
📒 Files selected for processing (2)
src/sampleworks/utils/guidance_script_arguments.pytests/utils/test_guidance_script_arguments.py
✅ Files skipped from review due to trivial changes (1)
- src/sampleworks/utils/guidance_script_arguments.py
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Abdelsalam-Abbas
had a problem deploying
to
gpu-testing
GitHub Actions
Error
When guidance runs inside Docker, job_metadata.json recorded container-internal paths (/data/inputs/..., /data/results/...) instead of host paths, making it impossible to reproduce experiments from metadata alone. Add container-to-host path remapping in GuidanceConfig.as_dict() via env vars: - SAMPLEWORKS_HOST_INPUT_DIR replaces /data/inputs - SAMPLEWORKS_HOST_RESULTS_DIR replaces /data/results - SAMPLEWORKS_HOST_DIR replaces /data (fallback for single-mount setups) Remapping only affects metadata serialization; runtime file I/O is unchanged. When no env vars are set, behavior is identical to before.
Abdelsalam-Abbas
force-pushed
the
fix-metadata-host-paths
branch
from
6f36301 to
a5848eb
Compare
Abdelsalam-Abbas
requested a deployment
to
gpu-testing
GitHub Actions
Waiting
Abdelsalam-Abbas
requested a deployment
to
gpu-testing
GitHub Actions
Waiting
Abdelsalam-Abbas
requested a deployment
to
gpu-testing
GitHub Actions
Waiting
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Closes #210
Summary
job_metadata.jsonrecorded container-internal paths (/data/inputs/...,/data/results/...) instead of host paths, making it impossible to reproduce experiments from metadata alone_remap_container_path()inGuidanceConfig.as_dict()that substitutes container path prefixes with host paths using env varsSAMPLEWORKS_HOST_INPUT_DIR(/data/inputs),SAMPLEWORKS_HOST_RESULTS_DIR(/data/results),SAMPLEWORKS_HOST_DIR(/datacatch-all for single mounts)Test plan
_remap_container_path: no-op without env vars, split mount remapping, catch-all remapping, specificity priority, checkpoint passthrough, trailing slash normalization, exact prefix match, empty env var handlingas_dict(): split mounts, catch-all, and no-env-var backward compatpixi run -e boltz-dev tests -- tests/utils/test_guidance_script_arguments.py -v(39 passed)Summary by CodeRabbit
New Features
Chores
Documentation
Tests