fix OpenAPI Context #524
Closed
fix OpenAPI Context #524
Conversation
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
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughThe changes ensure the existence of a server home directory at module load in Changes
Sequence Diagram(s)sequenceDiagram
participant Module as openapi_context.py (module load)
participant OS as os module
participant Utils as unzip_file / zip_file_list
Module->>OS: os.makedirs(DP_CLOUD_SERVER_HOME_DIR, exist_ok=True)
Note right of Module: Ensures directory exists at import
Module->>Utils: Defines unzip_file and zip_file_list functions
Note right of Utils: Functions available for later use
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
dpdispatcher/contexts/openapi_context.py (1)
76-101: Consider refactoring credential retrieval logic to reduce duplication.The credential retrieval and validation logic is duplicated between
openapi.pyandopenapi_context.py.Consider creating a utility function to handle credential retrieval:
# In a shared utility module def get_bohrium_credentials(remote_profile): """Retrieve Bohrium credentials from profile or environment.""" access_key = ( remote_profile.get("access_key", None) or os.getenv("BOHRIUM_ACCESS_KEY", None) or os.getenv("ACCESS_KEY", None) ) project_id = ( remote_profile.get("project_id", None) or os.getenv("BOHRIUM_PROJECT_ID", None) or os.getenv("PROJECT_ID", None) ) app_key = ( remote_profile.get("app_key", None) or os.getenv("BOHRIUM_APP_KEY", None) or os.getenv("APP_KEY", None) ) if access_key is None: raise ValueError( "remote_profile must contain 'access_key' or set environment variable 'BOHRIUM_ACCESS_KEY'" ) if project_id is None: raise ValueError( "remote_profile must contain 'project_id' or set environment variable 'BOHRIUM_PROJECT_ID'" ) return access_key, project_id, app_key
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dpdispatcher/contexts/openapi_context.py(5 hunks)dpdispatcher/machines/openapi.py(4 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
dpdispatcher/machines/openapi.py (5)
dpdispatcher/utils/utils.py (1)
customized_script_header_template(202-207)dpdispatcher/machines/dp_cloud_server.py (1)
Bohrium(19-299)dpdispatcher/submission.py (1)
Job(725-939)dpdispatcher/contexts/openapi_context.py (1)
unzip_file(29-32)dpdispatcher/utils/dpcloudserver/zip_file.py (1)
unzip_file(80-83)
🔇 Additional comments (1)
dpdispatcher/machines/openapi.py (1)
44-70: Well-implemented credential handling with proper validation.The explicit credential retrieval with multiple fallback sources and validation is a good security practice. The error messages are clear and helpful.
Comment on lines
23
to
+26
| DP_CLOUD_SERVER_HOME_DIR = os.path.join( | ||
| os.path.expanduser("~"), ".dpdispatcher/", "dp_cloud_server/" | ||
| ) | ||
| os.makedirs(DP_CLOUD_SERVER_HOME_DIR, exist_ok=True) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid creating directories at module import time.
Creating directories during module import can cause issues in restricted environments. Consider creating the directory lazily when first needed.
Move the directory creation to methods that actually use it, or create a helper function:
def ensure_home_dir_exists():
os.makedirs(DP_CLOUD_SERVER_HOME_DIR, exist_ok=True)Then call this function in methods like write_home_file and read_home_file.
🤖 Prompt for AI Agents
In dpdispatcher/contexts/openapi_context.py around lines 23 to 26, the directory
is created at module import time, which can cause issues in restricted
environments. Remove the os.makedirs call from the module level and instead
create a helper function that ensures the directory exists. Call this helper
function inside methods that use the directory, such as write_home_file and
read_home_file, to create the directory lazily when first needed.
Comment on lines
29
to
32
| def unzip_file(zip_file, out_dir="./"): | ||
| obj = ZipFile(zip_file, "r") | ||
| for item in obj.namelist(): | ||
| obj.extract(item, out_dir) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate code duplication by creating a shared utility module.
The unzip_file function is duplicated in both openapi.py and openapi_context.py. This violates the DRY principle.
Create a shared utility module (e.g., dpdispatcher/utils/zip_utils.py) and move both unzip_file and zip_file_list functions there:
# dpdispatcher/utils/zip_utils.py
from zipfile import ZipFile
import glob
import os
from dpdispatcher.dlog import dlog
def unzip_file(zip_file, out_dir="./"):
"""Extract zip file to specified directory."""
try:
with ZipFile(zip_file, "r") as obj:
for item in obj.namelist():
obj.extract(item, out_dir)
except Exception as e:
dlog.error(f"Failed to unzip file {zip_file}: {str(e)}")
raise
def zip_file_list(root_path, zip_filename, file_list=[]):
"""Create zip file from list of files."""
# ... existing implementation with error handling ...Then import and use these functions in both files.
🤖 Prompt for AI Agents
In dpdispatcher/contexts/openapi_context.py around lines 29 to 32, the
unzip_file function duplicates code found in openapi.py, violating the DRY
principle. To fix this, create a new shared utility module at
dpdispatcher/utils/zip_utils.py, move both unzip_file and zip_file_list
functions there with proper error handling and logging, then update both
openapi_context.py and openapi.py to import and use these functions from the new
utility module.
Comment on lines
35
to
56
| def zip_file_list(root_path, zip_filename, file_list=[]): | ||
| out_zip_file = os.path.join(root_path, zip_filename) | ||
| # print('debug: file_list', file_list) | ||
| zip_obj = ZipFile(out_zip_file, "w") | ||
| for f in file_list: | ||
| matched_files = os.path.join(root_path, f) | ||
| for ii in glob.glob(matched_files): | ||
| # print('debug: matched_files:ii', ii) | ||
| if os.path.isdir(ii): | ||
| arcname = os.path.relpath(ii, start=root_path) | ||
| zip_obj.write(ii, arcname) | ||
| for root, dirs, files in os.walk(ii): | ||
| for file in files: | ||
| filename = os.path.join(root, file) | ||
| arcname = os.path.relpath(filename, start=root_path) | ||
| # print('debug: filename:arcname:root_path', filename, arcname, root_path) | ||
| zip_obj.write(filename, arcname) | ||
| else: | ||
| arcname = os.path.relpath(ii, start=root_path) | ||
| zip_obj.write(ii, arcname) | ||
| zip_obj.close() | ||
| return out_zip_file |
There was a problem hiding this comment.
Fix mutable default argument and add error handling.
The function has several issues that need addressing:
- Mutable default argument can cause unexpected behavior
- Debug print statements should be removed
- Missing error handling
- Missing context manager for ZipFile
-def zip_file_list(root_path, zip_filename, file_list=[]):
+def zip_file_list(root_path, zip_filename, file_list=None):
+ if file_list is None:
+ file_list = []
out_zip_file = os.path.join(root_path, zip_filename)
- # print('debug: file_list', file_list)
- zip_obj = ZipFile(out_zip_file, "w")
- for f in file_list:
- matched_files = os.path.join(root_path, f)
- for ii in glob.glob(matched_files):
- # print('debug: matched_files:ii', ii)
- if os.path.isdir(ii):
- arcname = os.path.relpath(ii, start=root_path)
- zip_obj.write(ii, arcname)
- for root, dirs, files in os.walk(ii):
- for file in files:
- filename = os.path.join(root, file)
- arcname = os.path.relpath(filename, start=root_path)
- # print('debug: filename:arcname:root_path', filename, arcname, root_path)
- zip_obj.write(filename, arcname)
- else:
- arcname = os.path.relpath(ii, start=root_path)
- zip_obj.write(ii, arcname)
- zip_obj.close()
- return out_zip_file
+ try:
+ with ZipFile(out_zip_file, "w") as zip_obj:
+ for f in file_list:
+ matched_files = os.path.join(root_path, f)
+ for ii in glob.glob(matched_files):
+ if os.path.isdir(ii):
+ arcname = os.path.relpath(ii, start=root_path)
+ zip_obj.write(ii, arcname)
+ for root, dirs, files in os.walk(ii):
+ for file in files:
+ filename = os.path.join(root, file)
+ arcname = os.path.relpath(filename, start=root_path)
+ zip_obj.write(filename, arcname)
+ else:
+ arcname = os.path.relpath(ii, start=root_path)
+ zip_obj.write(ii, arcname)
+ return out_zip_file
+ except Exception as e:
+ dlog.error(f"Failed to create zip file {out_zip_file}: {str(e)}")
+ raise📝 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.
Suggested change
| def zip_file_list(root_path, zip_filename, file_list=[]): | |
| out_zip_file = os.path.join(root_path, zip_filename) | |
| # print('debug: file_list', file_list) | |
| zip_obj = ZipFile(out_zip_file, "w") | |
| for f in file_list: | |
| matched_files = os.path.join(root_path, f) | |
| for ii in glob.glob(matched_files): | |
| # print('debug: matched_files:ii', ii) | |
| if os.path.isdir(ii): | |
| arcname = os.path.relpath(ii, start=root_path) | |
| zip_obj.write(ii, arcname) | |
| for root, dirs, files in os.walk(ii): | |
| for file in files: | |
| filename = os.path.join(root, file) | |
| arcname = os.path.relpath(filename, start=root_path) | |
| # print('debug: filename:arcname:root_path', filename, arcname, root_path) | |
| zip_obj.write(filename, arcname) | |
| else: | |
| arcname = os.path.relpath(ii, start=root_path) | |
| zip_obj.write(ii, arcname) | |
| zip_obj.close() | |
| return out_zip_file | |
| def zip_file_list(root_path, zip_filename, file_list=None): | |
| if file_list is None: | |
| file_list = [] | |
| out_zip_file = os.path.join(root_path, zip_filename) | |
| try: | |
| with ZipFile(out_zip_file, "w") as zip_obj: | |
| for f in file_list: | |
| matched_files = os.path.join(root_path, f) | |
| for ii in glob.glob(matched_files): | |
| if os.path.isdir(ii): | |
| arcname = os.path.relpath(ii, start=root_path) | |
| zip_obj.write(ii, arcname) | |
| for root, dirs, files in os.walk(ii): | |
| for file in files: | |
| filename = os.path.join(root, file) | |
| arcname = os.path.relpath(filename, start=root_path) | |
| zip_obj.write(filename, arcname) | |
| else: | |
| arcname = os.path.relpath(ii, start=root_path) | |
| zip_obj.write(ii, arcname) | |
| return out_zip_file | |
| except Exception as e: | |
| dlog.error(f"Failed to create zip file {out_zip_file}: {e}") | |
| raise |
🤖 Prompt for AI Agents
In dpdispatcher/contexts/openapi_context.py lines 35 to 56, fix the mutable
default argument by changing file_list's default to None and initializing it
inside the function. Remove all debug print statements. Add error handling
around the file zipping process to catch and log exceptions. Use a context
manager (with statement) for the ZipFile object to ensure it is properly closed
even if errors occur.
dpdispatcher/machines/openapi.py
Outdated
Comment on lines
25
to
29
| def unzip_file(zip_file, out_dir="./"): | ||
| obj = ZipFile(zip_file, "r") | ||
| for item in obj.namelist(): | ||
| obj.extract(item, out_dir) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for zip file operations.
The unzip_file function should handle potential errors such as corrupted zip files or extraction failures.
def unzip_file(zip_file, out_dir="./"):
- obj = ZipFile(zip_file, "r")
- for item in obj.namelist():
- obj.extract(item, out_dir)
+ try:
+ with ZipFile(zip_file, "r") as obj:
+ for item in obj.namelist():
+ obj.extract(item, out_dir)
+ except Exception as e:
+ dlog.error(f"Failed to unzip file {zip_file}: {str(e)}")
+ raise📝 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.
Suggested change
| def unzip_file(zip_file, out_dir="./"): | |
| obj = ZipFile(zip_file, "r") | |
| for item in obj.namelist(): | |
| obj.extract(item, out_dir) | |
| def unzip_file(zip_file, out_dir="./"): | |
| try: | |
| with ZipFile(zip_file, "r") as obj: | |
| for item in obj.namelist(): | |
| obj.extract(item, out_dir) | |
| except Exception as e: | |
| dlog.error(f"Failed to unzip file {zip_file}: {str(e)}") | |
| raise |
🤖 Prompt for AI Agents
In dpdispatcher/machines/openapi.py around lines 25 to 29, the unzip_file
function lacks error handling for zip file operations. Add try-except blocks
around the ZipFile opening and extraction code to catch exceptions like
BadZipFile or extraction errors. Log or raise appropriate errors to handle
corrupted zip files or extraction failures gracefully.
pyproject.toml
Outdated
Comment on lines
32
to
35
| 'httpx', | ||
| 'distro', | ||
| 'pyhumps', | ||
| 'bohrium-sdk' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant bohrium-sdk from optional dependencies.
Since bohrium-sdk is now a required dependency (line 35), it should be removed from the optional dependencies to avoid redundancy.
-cloudserver = ["oss2", "tqdm", "bohrium-sdk"]
-bohrium = ["oss2", "tqdm", "bohrium-sdk"]
+cloudserver = ["oss2", "tqdm"]
+bohrium = ["oss2", "tqdm"]Also applies to: 64-65
🤖 Prompt for AI Agents
In pyproject.toml around lines 32 to 35 and also lines 64 to 65, remove
'bohrium-sdk' from the optional dependencies section because it is already
listed as a required dependency on line 35. This will eliminate redundancy and
keep the dependency declarations clear.
🛠️ Refactor suggestion
Consider adding version constraints for new dependencies.
The newly added dependencies lack version constraints, which could lead to compatibility issues in the future. Consider pinning versions or specifying minimum versions for stability.
- 'httpx',
- 'distro',
- 'pyhumps',
- 'bohrium-sdk'
+ 'httpx>=0.24.0',
+ 'distro>=1.8.0',
+ 'pyhumps>=3.8.0',
+ 'bohrium-sdk>=0.1.0'📝 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.
Suggested change
| 'httpx', | |
| 'distro', | |
| 'pyhumps', | |
| 'bohrium-sdk' | |
| 'httpx>=0.24.0', | |
| 'distro>=1.8.0', | |
| 'pyhumps>=3.8.0', | |
| 'bohrium-sdk>=0.1.0' |
🤖 Prompt for AI Agents
In pyproject.toml around lines 32 to 35, the newly added dependencies 'httpx',
'distro', 'pyhumps', and 'bohrium-sdk' do not have version constraints
specified. To fix this, update the dependency entries to include either exact
versions or minimum version requirements to ensure compatibility and stability.
Check the latest stable versions of these packages and add appropriate version
specifiers such as '>=x.y.z' or '==x.y.z' for each dependency.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #524 +/- ##
==========================================
- Coverage 47.73% 47.43% -0.30%
==========================================
Files 39 39
Lines 3863 3904 +41
==========================================
+ Hits 1844 1852 +8
- Misses 2019 2052 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
njzjz
requested changes
Jul 24, 2025
Comment on lines
+32
to
+34
| 'httpx', | ||
| 'distro', | ||
| 'pyhumps' |
There was a problem hiding this comment.
These should be claimed in bohrium-sdk. I don't think they should be claimed in dpdispatcher
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by CodeRabbit
New Features
Chores