Compatible for bohrium OpenAPI sandbox #553
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Signed-off-by: zjgemi <[email protected]>
Signed-off-by: zjgemi <[email protected]>
Signed-off-by: zjgemi <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #553 +/- ##
==========================================
- Coverage 60.40% 54.83% -5.58%
==========================================
Files 39 40 +1
Lines 3905 3921 +16
==========================================
- Hits 2359 2150 -209
- Misses 1546 1771 +225 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughSafer jobGroupId retrieval in OpenAPI context; rename of image parameter and conditional inclusion of remote_profile fields in submit flow; gate log retrieval on output_log and switch download URL to Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OpenAPI_Machine
participant OpenAPI_Service
rect rgb(230, 245, 255)
Client->>OpenAPI_Machine: do_submit(image_name, job data)
Note right of OpenAPI_Machine: include real_user_id/session_id\nonly if remote_profile present
OpenAPI_Machine->>OpenAPI_Service: submit job (openapi_params with job_id)
OpenAPI_Service-->>OpenAPI_Machine: job accepted / job_id
end
rect rgb(245, 255, 230)
Client->>OpenAPI_Machine: check_status(job_id)
OpenAPI_Machine->>OpenAPI_Service: query status
OpenAPI_Service-->>OpenAPI_Machine: status (e.g., -1 / finished / running)
alt status finished
Note right of OpenAPI_Machine: if output_log == true -> fetch logs
OpenAPI_Machine->>OpenAPI_Service: request logs (conditional)
OpenAPI_Service-->>OpenAPI_Machine: logs
OpenAPI_Machine->>OpenAPI_Service: request result URL
OpenAPI_Service-->>OpenAPI_Machine: `resultUrl`
OpenAPI_Machine->>Client: download result from `resultUrl`
else status -1 and ignore_exit_code == true
OpenAPI_Machine->>Client: mark job finished
else other statuses
OpenAPI_Machine->>Client: report current status
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dpdispatcher/machines/openapi.py (1)
232-251: Remove unusedexit_codeparameter.The
exit_codeparameter is no longer used in the method body after removing theexit_code != 0check on line 249. This makes the parameter misleading.Apply this diff to remove the unused parameter:
@staticmethod - def map_dp_job_state(status, exit_code, ignore_exit_code=True): + def map_dp_job_state(status, ignore_exit_code=True): if isinstance(status, JobStatus): return status map_dict = {Then update the caller on line 182-186:
job_state = self.map_dp_job_state( dp_job_status, - check_return.get("exitCode", 0), # type: ignore self.ignore_exit_code, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dpdispatcher/contexts/openapi_context.py(2 hunks)dpdispatcher/machines/openapi.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Always add type hints in all Python code
Ensure code remains compatible with Python 3.7+ (avoid syntax/features newer than 3.7)
Use Ruff (via pre-commit) for linting and formatting
Files:
dpdispatcher/contexts/openapi_context.pydpdispatcher/machines/openapi.py
🧬 Code graph analysis (1)
dpdispatcher/machines/openapi.py (2)
dpdispatcher/contexts/ssh_context.py (1)
get(432-448)dpdispatcher/utils/dpcloudserver/client.py (1)
get(47-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build (ssh_rsync)
- GitHub Check: build (pbs)
- GitHub Check: build (slurm)
🔇 Additional comments (3)
dpdispatcher/contexts/openapi_context.py (1)
168-174: Good defensive coding, but add type hints.Using
.get()with a default preventsKeyErrorif the key is missing. However, per the coding guidelines, type hints should be added throughout the codebase.Consider adding type annotations to the
jgidattributes where they are declared (e.g., in class definitions).As per coding guidelines
dpdispatcher/machines/openapi.py (2)
188-190: LGTM!Gating log retrieval behind the
output_logflag avoids unnecessary API calls when logs are not needed, improving performance.
132-138: No action needed—the original review comment is incorrect.The code at lines 132-138 is safe and intentional:
Line 138 (
job.job_id): Thejob.job_idattribute is initialized to""(empty string) inJob.__init__(line 752 of submission.py). It is always defined beforedo_submitis called, so there is no risk of uninitialized access. On first submission, it is""; on resubmission, it holds the previous job ID—both are valid.Line 132 (
image_address→image_name): This mapping is intentional. The remote_profile uses the key"image_address"as configuration, while the OpenAPI backend expects the parameter key"image_name". This is confirmed by test cases (test_run_submission_bohrium.py) where different machine types use different key names, and the code correctly bridges them.Likely an incorrect or invalid review comment.
|
Is Ready for merge? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dpdispatcher/contexts/openapi_context.py (1)
139-178: Consider adding type hints to the method signature.The coding guidelines require type hints in all Python code, but the
upload_jobmethod lacks them. Adding type annotations would improve maintainability and enable better static analysis.Example type hints:
def upload_job(self, job, common_files: list[str] | None = None) -> None:Note: For Python 3.7-3.9 compatibility, use
Optional[List[str]]fromtypinginstead oflist[str] | None.As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dpdispatcher/contexts/openapi_context.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Always add type hints in all Python code
Ensure code remains compatible with Python 3.7+ (avoid syntax/features newer than 3.7)
Use Ruff (via pre-commit) for linting and formatting
Files:
dpdispatcher/contexts/openapi_context.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pass
- GitHub Check: build (pbs)
- GitHub Check: build (ssh_rsync)
- GitHub Check: build (slurm)
🔇 Additional comments (1)
dpdispatcher/contexts/openapi_context.py (1)
173-173: Safer jobGroupId retrieval approved.Good change to use
.get()with a default empty string, preventing potential KeyError if the API response omits this field.Please verify that an empty string is the appropriate default for
jgid. Consider whether downstream code expects a specific sentinel value (e.g.,None) when no job group exists.
| object_key = os.path.join(data["storePath"], zip_filename) # type: ignore | ||
| job.upload_path = object_key | ||
| job.job_id = data["jobId"] # type: ignore |
There was a problem hiding this comment.
Inconsistent error handling for API response fields.
Lines 170 and 172 use direct indexing (data["storePath"], data["jobId"]) while lines 167, 168, and 173 use safe .get() access. If the API can omit jobGroupId, it may also omit storePath or jobId, leading to KeyError at runtime.
Apply this diff to use consistent safe access:
- object_key = os.path.join(data["storePath"], zip_filename) # type: ignore
+ object_key = os.path.join(data.get("storePath", ""), zip_filename) # type: ignore
job.upload_path = object_key
- job.job_id = data["jobId"] # type: ignore
+ job.job_id = data.get("jobId", "") # type: ignore📝 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.
| object_key = os.path.join(data["storePath"], zip_filename) # type: ignore | |
| job.upload_path = object_key | |
| job.job_id = data["jobId"] # type: ignore | |
| object_key = os.path.join(data.get("storePath", ""), zip_filename) # type: ignore | |
| job.upload_path = object_key | |
| job.job_id = data.get("jobId", "") # type: ignore |
🤖 Prompt for AI Agents
In dpdispatcher/contexts/openapi_context.py around lines 170 to 172, replace the
direct indexing data["storePath"] and data["jobId"] with safe .get() access
consistent with the surrounding code (e.g., data.get("storePath") and
data.get("jobId")); then handle the possibility of None by either raising a
clear ValueError or logging an error and providing a sensible default (or
aborting the operation) so a missing field does not cause an unhandled KeyError
at runtime.
4 participants
Summary by CodeRabbit
Bug Fixes
Improvements