Hide docker and make conversion rootles - #262
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe PR replaces the shared project directory workflow with a hidden cache and local ChangesCache-backed Docker conversion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant InputCache
participant DockerCompose
participant ModelConverter
participant OutputDirectory
CLI->>InputCache: Stage local inputs by content hash
CLI->>DockerCompose: Start with rewritten cache paths
DockerCompose->>ModelConverter: Run conversion or inference
ModelConverter->>OutputDirectory: Write artifacts and logs
DockerCompose-->>CLI: Restore output ownership and exit status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modelconverter/cli/utils.py (1)
90-108: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset
_input_baseat the start ofget_configs.
set_input_basesets aContextVar, but this function never callsset_input_base(None). In the CLI entrypoints,convertreturns whileinfercontinues in the same process; if the first config file leaves a non-Nonebase, later resolution ininfercan use that stale custom directory instead of the default cache/cwd base.🛡️ Proposed fix: reset the base unconditionally at the top of the function
opts = opts or [] + # Reset to the default resolution base so state left over from a + # previous call in this process does not leak into this one. + set_input_base(None) if isinstance(opts, list): if len(opts) % 2 != 0: raise ValueError( "Invalid number of overrides. See --help for more information." )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelconverter/cli/utils.py` around lines 90 - 108, Reset the input base unconditionally at the start of get_configs by calling set_input_base(None) before processing opts or path. Preserve the existing set_input_base(path_.parent) behavior for config-file-relative resolution and all subsequent archive/config handling.
🧹 Nitpick comments (6)
modelconverter/utils/docker_utils.py (1)
118-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winContainer mount path duplicated across files.
/app/shared_with_containerhere duplicates the same literal inconstants.py,input_staging.py, anddocker-compose.yaml. See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelconverter/utils/docker_utils.py` around lines 118 - 119, Replace the hardcoded /app/shared_with_container mount path in the Docker volume configuration with the shared constant defined in constants.py, updating the relevant docker utility symbol while preserving the existing host-path mapping.modelconverter/utils/constants.py (1)
26-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winContainer mount path duplicated across files.
"/app/shared_with_container"is hardcoded here and re-hardcoded ininput_staging.py,docker_utils.py, anddocker-compose.yaml. If the mount point ever changes, all four sites must be updated in lockstep. See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelconverter/utils/constants.py` around lines 26 - 31, Centralize the container mount path used by SHARED_DIR in a single shared constant, then update input_staging.py, docker_utils.py, and docker-compose.yaml to reference that same configuration value instead of hardcoding "/app/shared_with_container". Preserve the existing Docker and non-Docker directory behavior.modelconverter/utils/input_staging.py (2)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded container mount path, duplicated across files.
_CONTAINER_SHARED_DIR = Path("/app/shared_with_container")duplicates the same literal inconstants.py,docker_utils.py, anddocker-compose.yaml. See consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelconverter/utils/input_staging.py` at line 25, Replace the hardcoded path in _CONTAINER_SHARED_DIR with the shared constant defined in constants.py, and update the related references in docker_utils.py and docker-compose.yaml to use the same centralized value where applicable. Preserve the existing /app/shared_with_container behavior while eliminating duplicated literals.
141-146: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftStaging a config's whole parent directory can sweep up unrelated files.
Because config-relative references resolve against the copied directory,
_stage_valuecopiessrc.parentin full for any.yaml/.ymlconfig. If the config lives directly in a broad directory (home dir,Downloads/, a shared project root), the entire tree — including files unrelated to the conversion — gets copied into the persistent cache. The_LARGE_DIR_BYTESwarning only fires after the copy decision and doesn't scope what gets copied.Consider: only copying files actually referenced by the config (would require a lightweight scan of local-file fields), or requiring configs to live in a dedicated directory before auto-staging, or prompting before staging directories above a size/depth threshold.
Also applies to: 186-201
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelconverter/utils/input_staging.py` around lines 141 - 146, Update _stage_value’s config handling so it does not automatically stage the entire src.parent directory when a YAML config resides in a broad or oversized location. Restrict auto-staging to dedicated/safe config directories, or require explicit confirmation/handling for directories exceeding the size or depth threshold, while preserving relative-reference resolution for accepted configs and avoiding unrelated files in the persistent cache.modelconverter/__main__.py (1)
802-815: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
cache cleanhas no confirmation before deleting downloaded models/datasets.
cache_clean()immediatelyshutil.rmtrees the entire cache, includingmodels/andcalibration_data/, which may be expensive to re-download. A confirmation prompt (or a--force/-yflag to skip it) would guard against accidental data loss from a single fat-fingered command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelconverter/__main__.py` around lines 802 - 815, Update cache_clean to require explicit confirmation before calling shutil.rmtree on the cache, while providing a force/yes option that bypasses the prompt for non-interactive use. Preserve the existing empty-cache behavior and success messaging, and ensure deletion only proceeds after confirmation or the force option is enabled.docker-compose.yaml (1)
8-14: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMissing rootless-Docker warning for manually-exported
HOST_UID/HOST_GID.This comment instructs users to
export HOST_UID=$(id -u) HOST_GID=$(id -g), butdocker_utils.py'sgenerate_compose_configdeliberately omits these variables under rootless Docker — its own docstring explains that chowning in rootless mode maps files to an unmapped sub-uid instead of the host user. A user who follows this file's instructions while running rootless Docker would hit exactly that breakage, with no warning here.Add a note (or an
id -u/rootless detection snippet) warning users on rootless Docker not to exportHOST_UID/HOST_GID.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.yaml` around lines 8 - 14, Update the HOST_UID/HOST_GID comment in docker-compose.yaml to warn users running rootless Docker not to export these variables, since chowning would map outputs to an unmapped sub-UID. Preserve the existing instructions for non-rootless Docker and keep the warning adjacent to the variable declarations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@configs/defaults.yaml`:
- Around line 168-170: Remove the redundant commented-out “# batch_size: 8” line
immediately before the active batch_size setting, leaving the calibration
batch_size configuration unchanged.
- Around line 42-58: Update the calibration comments near the
stage/output/script fields to describe cross-stage linking as flat sibling
properties under calibration, not as a nested link section. Remove or revise the
“link section” wording while preserving the existing field descriptions and
required-field guidance.
In `@docker/hailo/entrypoint.sh`:
- Around line 15-35: Update the command execution flow in the entrypoint around
chown_mounts so the wrapper forwards termination signals to the running
/bin/bash or modelconverter child while still waiting for its exit status and
invoking chown_mounts afterward. Apply the same signal-handling and cleanup flow
to the duplicated rvc2 and rvc4 entrypoints, preserving the existing argument
selection and status propagation.
In `@docker/rvc2/entrypoint.sh`:
- Around line 9-29: Update the command execution paths in the entrypoint script
to preserve PID 1 signal forwarding by replacing the non-exec launches of bash
and modelconverter with exec-based execution. Keep the existing status capture
and chown_mounts cleanup behavior for both interactive and conversion modes,
ensuring cleanup still runs after the child exits.
In `@docker/rvc4/entrypoint.sh`:
- Around line 17-37: Update the command execution paths in the entrypoint script
to use exec for both the interactive bash branch and the modelconverter branch,
while preserving the existing exit-status and chown_mounts behavior needed after
normal completion. Ensure modelconverter or bash receives PID 1 signals directly
and chown_mounts still runs before exiting when the command terminates.
In `@modelconverter/utils/input_staging.py`:
- Around line 156-201: Make staging atomic in _stage_file, _stage_ir_pair, and
_stage_dir: copy each artifact into a uniquely named temporary sibling under the
digest directory, then publish it with os.replace only after copying completes.
For directories, use shutil.copytree on the temporary directory before replacing
the final directory; preserve existing cache-hit behavior while preventing
incomplete concurrent or interrupted copies from being treated as valid.
---
Outside diff comments:
In `@modelconverter/cli/utils.py`:
- Around line 90-108: Reset the input base unconditionally at the start of
get_configs by calling set_input_base(None) before processing opts or path.
Preserve the existing set_input_base(path_.parent) behavior for
config-file-relative resolution and all subsequent archive/config handling.
---
Nitpick comments:
In `@docker-compose.yaml`:
- Around line 8-14: Update the HOST_UID/HOST_GID comment in docker-compose.yaml
to warn users running rootless Docker not to export these variables, since
chowning would map outputs to an unmapped sub-UID. Preserve the existing
instructions for non-rootless Docker and keep the warning adjacent to the
variable declarations.
In `@modelconverter/__main__.py`:
- Around line 802-815: Update cache_clean to require explicit confirmation
before calling shutil.rmtree on the cache, while providing a force/yes option
that bypasses the prompt for non-interactive use. Preserve the existing
empty-cache behavior and success messaging, and ensure deletion only proceeds
after confirmation or the force option is enabled.
In `@modelconverter/utils/constants.py`:
- Around line 26-31: Centralize the container mount path used by SHARED_DIR in a
single shared constant, then update input_staging.py, docker_utils.py, and
docker-compose.yaml to reference that same configuration value instead of
hardcoding "/app/shared_with_container". Preserve the existing Docker and
non-Docker directory behavior.
In `@modelconverter/utils/docker_utils.py`:
- Around line 118-119: Replace the hardcoded /app/shared_with_container mount
path in the Docker volume configuration with the shared constant defined in
constants.py, updating the relevant docker utility symbol while preserving the
existing host-path mapping.
In `@modelconverter/utils/input_staging.py`:
- Line 25: Replace the hardcoded path in _CONTAINER_SHARED_DIR with the shared
constant defined in constants.py, and update the related references in
docker_utils.py and docker-compose.yaml to use the same centralized value where
applicable. Preserve the existing /app/shared_with_container behavior while
eliminating duplicated literals.
- Around line 141-146: Update _stage_value’s config handling so it does not
automatically stage the entire src.parent directory when a YAML config resides
in a broad or oversized location. Restrict auto-staging to dedicated/safe config
directories, or require explicit confirmation/handling for directories exceeding
the size or depth threshold, while preserving relative-reference resolution for
accepted configs and avoiding unrelated files in the persistent cache.
🪄 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 Plus
Run ID: 02dfb1c2-8742-46e5-9c1a-3353e17cc74c
📒 Files selected for processing (24)
.dockerignore.gitignoreREADME.mdconfigs/defaults.yamlconfigs/mnist.yamlconfigs/multistage.yamlconfigs/resnet18.yamlconfigs/resnet18_IR.yamlconfigs/yolov6n.yamlconfigs/yolov8n_seg.yamldocker-compose.yamldocker/hailo/entrypoint.shdocker/rvc2/entrypoint.shdocker/rvc4/entrypoint.shmodelconverter/__main__.pymodelconverter/cli/utils.pymodelconverter/utils/calibration_data.pymodelconverter/utils/constants.pymodelconverter/utils/docker_utils.pymodelconverter/utils/filesystem_utils.pymodelconverter/utils/input_staging.pytests/conftest.pytests/test_packages/common.pytests/test_utils/test_config.py
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| return process_nn_archive(target, path_, overrides) | ||
| # Resolve files referenced *inside* the config (calibration data, | ||
| # scripts, encodings, ...) relative to the config file's directory. | ||
| set_input_base(path_.parent) |
There was a problem hiding this comment.
Should this be called at function entry? Otherwise if path is not set we skip the reset of the input base
| # references resolve alongside it, then stage and rewrite absolute local | ||
| # references which may live anywhere on the host. | ||
| if src.suffix.lower() in _CONFIG_EXTS: | ||
| parent_dest = _stage_dir(src.parent, inputs_dir) |
There was a problem hiding this comment.
This could be very problematic if I for example have config in some big directory because in that case it would try to sync this whole big directory to the cache even though we don't really need any of it.
What if we would go the other way around: Check for files that we need to copy, copy only these files and rewrite the paths to these files to container specific paths for the specific modelconv process?
AI proposed solution:
Conceptually, replace this:
parent_dest = _stage_dir(src.parent, inputs_dir)
config_dest = parent_dest / src.namewith:
config_dest = _stage_file(src, inputs_dir)
_rewrite_config_paths(src, config_dest, src.parent, inputs_dir)_rewrite_config_paths would:
- Load the YAML.
- Walk only known path-bearing fields.
- Resolve relative local values against
src.parent. - Stage the resolved path using the existing
_stage_value. - Replace that YAML value with the returned
/app/shared_with_container/inputs/...path. - Write the rewritten YAML to the staged config location.
Illustrative shape:
_PATH_FIELDS = {"input_model", "input_bin", "path", "script", "encodings"}
def _rewrite_config_paths(value, *, config_dir, inputs_dir, key=None):
if isinstance(value, dict):
return {
field: _rewrite_config_paths(
item,
config_dir=config_dir,
inputs_dir=inputs_dir,
key=field,
)
for field, item in value.items()
}
if isinstance(value, list):
return [
_rewrite_config_paths(
item, config_dir=config_dir, inputs_dir=inputs_dir, key=key
)
for item in value
]
if key not in _PATH_FIELDS or not isinstance(value, str):
return value
if get_protocol(value) != "file":
return value
candidate = Path(value).expanduser()
if not candidate.is_absolute():
candidate = config_dir / candidate
if not candidate.exists():
return value # Let normal config validation report the missing path.
return _stage_value(str(candidate.resolve()), inputs_dir) or valueIn the real implementation, I would scope path specifically to calibration.path, rather than treating every YAML field named path as a filesystem path. The supported fields should be defined from the config schema and covered by tests for:
- relative model, calibration directory, script, encodings, and OpenVINO
.bin; - absolute dependencies outside the config directory;
- remote URLs left unchanged;
- unrelated sibling files never copied.
2 participants
Purpose
Makes it much easier to use
modelconverterwithout managing shared directory with containers and dealing with root being the owner of converted files.Specification
shared_with_container/modelconverter convert rvc4 --path ~/Downloads/resnet18.onnx.tar.xzoutput/in CWDmodelconverter cachecommands to inspect and purge itDependencies & Potential Impact
None / not applicable
Deployment Plan
None / not applicable
Testing & Validation
None / not applicable
AI Usage
Assisted-by: AGENT_NAME:MODEL_VERSION [TOOL1] [TOOL2]
Submitted code was reviewed by a human: YES/NO
The author is taking the responsibility for the contribution: YES/NO
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests