-
Notifications
You must be signed in to change notification settings - Fork 57
update bohrium-sdk version 0.6.0 #520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughThe updates transition the codebase from the older Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OpenAPIContext
participant Bohrium
participant Tiefblue
User->>OpenAPIContext: Initialize(local_root, remote_profile)
OpenAPIContext->>OpenAPIContext: Retrieve access_key, project_id, app_key
OpenAPIContext->>Bohrium: Initialize(access_key, project_id, app_key)
OpenAPIContext->>Tiefblue: Initialize()
sequenceDiagram
participant User
participant OpenAPIContext
participant Tiefblue
User->>OpenAPIContext: upload_job(job, common_files)
OpenAPIContext->>OpenAPIContext: zip_file_list(...)
OpenAPIContext->>Tiefblue: Upload zip archive
sequenceDiagram
participant User
participant OpenAPIContext
participant Tiefblue
User->>OpenAPIContext: download(submission)
OpenAPIContext->>Tiefblue: Download zip archive
OpenAPIContext->>OpenAPIContext: unzip_file(...)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 GitHub Check: codecov/patchdpdispatcher/contexts/openapi_context.py[warning] 11-11: dpdispatcher/contexts/openapi_context.py#L11 [warning] 17-17: dpdispatcher/contexts/openapi_context.py#L17 [warning] 29-31: dpdispatcher/contexts/openapi_context.py#L29-L31 [warning] 35-35: dpdispatcher/contexts/openapi_context.py#L35 [warning] 37-40: dpdispatcher/contexts/openapi_context.py#L37-L40 [warning] 42-48: dpdispatcher/contexts/openapi_context.py#L42-L48 [warning] 50-50: dpdispatcher/contexts/openapi_context.py#L50 [warning] 52-55: dpdispatcher/contexts/openapi_context.py#L52-L55 [warning] 75-75: dpdispatcher/contexts/openapi_context.py#L75 [warning] 80-80: dpdispatcher/contexts/openapi_context.py#L80 [warning] 85-85: dpdispatcher/contexts/openapi_context.py#L85 [warning] 90-91: dpdispatcher/contexts/openapi_context.py#L90-L91 [warning] 94-95: dpdispatcher/contexts/openapi_context.py#L94-L95 [warning] 98-98: dpdispatcher/contexts/openapi_context.py#L98 [warning] 101-101: dpdispatcher/contexts/openapi_context.py#L101 [warning] 156-156: dpdispatcher/contexts/openapi_context.py#L156 [warning] 248-248: dpdispatcher/contexts/openapi_context.py#L248 🪛 Pylint (3.3.7)dpdispatcher/contexts/openapi_context.py[refactor] 29-29: Consider using 'with' for resource-allocating operations (R1732) [refactor] 37-37: Consider using 'with' for resource-allocating operations (R1732) ⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (5)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
dpdispatcher/machines/openapi.py (1)
44-63
: Credential lookup logic is solid – consider deduplicating & centralisingNice explicit precedence (
remote_profile
→ Bohrium-specific env → legacy env).
To avoid repetition across modules, you could move this snippet to a smallcredentials.py
helper and re-use it here and inopenapi_context.py
.dpdispatcher/contexts/openapi_context.py (1)
73-93
: Credential retrieval duplicated – extract to helper or mixinExactly the same block exists in
machines/openapi.py
; factor it out to reduce maintenance cost and ensure consistent behaviour.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dpdispatcher/contexts/openapi_context.py
(5 hunks)dpdispatcher/machines/openapi.py
(4 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
dpdispatcher/machines/openapi.py
[refactor] 26-26: Consider using 'with' for resource-allocating operations
(R1732)
dpdispatcher/contexts/openapi_context.py
[refactor] 27-27: Consider using 'with' for resource-allocating operations
(R1732)
[refactor] 35-35: Consider using 'with' for resource-allocating operations
(R1732)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a context-manager & path-sanitisation in unzip_file
The helper leaks the ZipFile
handle and is vulnerable to Zip-Slip because arbitrary paths from the archive are blindly extracted.
-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: str = "./"):
+ """Safely extract *zip_file* into *out_dir*, guarding against Zip-Slip."""
+ abs_out = os.path.abspath(out_dir)
+ with ZipFile(zip_file, "r") as zf:
+ for member in zf.namelist():
+ target = os.path.abspath(os.path.join(abs_out, member))
+ if not target.startswith(abs_out + os.sep):
+ raise ValueError(f"Unsafe path detected in zip: {member}")
+ zf.extractall(abs_out)
Closes the file handle automatically and blocks directory-traversal attacks.
📝 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.
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: str = "./"): | |
"""Safely extract *zip_file* into *out_dir*, guarding against Zip-Slip.""" | |
abs_out = os.path.abspath(out_dir) | |
with ZipFile(zip_file, "r") as zf: | |
for member in zf.namelist(): | |
target = os.path.abspath(os.path.join(abs_out, member)) | |
if not target.startswith(abs_out + os.sep): | |
raise ValueError(f"Unsafe path detected in zip: {member}") | |
zf.extractall(abs_out) |
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 26-26: Consider using 'with' for resource-allocating operations
(R1732)
🤖 Prompt for AI Agents
In dpdispatcher/machines/openapi.py around lines 25 to 30, the unzip_file
function currently opens a ZipFile without using a context manager, causing a
file handle leak, and extracts files without sanitizing paths, making it
vulnerable to Zip-Slip attacks. Fix this by using a with statement to open the
ZipFile, ensuring it closes automatically, and sanitize each extracted file path
to prevent directory traversal before extraction.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Same Zip concerns as in machines/openapi.py
Replicate the safer, context-managed implementation suggested earlier to avoid handle leaks and Zip-Slip.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 27-27: Consider using 'with' for resource-allocating operations
(R1732)
🤖 Prompt for AI Agents
In dpdispatcher/contexts/openapi_context.py around lines 26 to 30, the
unzip_file function opens the ZipFile without using a context manager and
extracts files without validating paths, risking resource leaks and Zip-Slip
attacks. Refactor the function to use a with statement to open the ZipFile,
ensuring it is properly closed, and add path validation to prevent extraction
outside the target directory, mitigating Zip-Slip vulnerabilities.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zip_file_list
– avoid mutable default & close the archive properly
Two issues:
file_list=[]
is a mutable default shared between calls.ZipFile
is not used in awith
block; handle remains open on exceptions.
-from zipfile import ZipFile
+from zipfile import ZipFile, ZIP_DEFLATED
@@
-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 = []
@@
- zip_obj = ZipFile(out_zip_file, "w")
- for f in file_list:
+ with ZipFile(out_zip_file, "w", compression=ZIP_DEFLATED) as zip_obj:
+ for f in file_list:
@@
- zip_obj.write(ii, arcname)
- zip_obj.close()
- return out_zip_file
+ zip_obj.write(ii, arcname)
+ return out_zip_file
This removes the shared-state bug, guarantees the file is flushed/closed, and activates compression for a smaller upload.
📝 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.
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 | |
from zipfile import ZipFile, ZIP_DEFLATED | |
import os | |
import glob | |
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) | |
with ZipFile(out_zip_file, "w", compression=ZIP_DEFLATED) 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 |
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 35-35: Consider using 'with' for resource-allocating operations
(R1732)
🤖 Prompt for AI Agents
In dpdispatcher/contexts/openapi_context.py around lines 32 to 53, the function
zip_file_list uses a mutable default argument file_list=[] which can cause
shared state bugs across calls, and it opens the ZipFile without a with
statement, risking the file not being closed properly on exceptions. To fix
this, change the default value of file_list to None and initialize it to an
empty list inside the function if None. Also, open the ZipFile using a with
statement to ensure it is properly closed after writing, and enable compression
by specifying the compression mode when creating the ZipFile.
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #520 +/- ##
===========================================
- Coverage 60.16% 47.42% -12.74%
===========================================
Files 39 39
Lines 3863 3903 +40
===========================================
- Hits 2324 1851 -473
- Misses 1539 2052 +513 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I print the detailed error in the unit test, and it is ModuleNotFoundError: No module named 'httpx'
ERROR: test_failed_submission (tests.test_run_submission_bohrium.TestOpenAPIRun.test_failed_submission)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/runner/work/dpdispatcher/dpdispatcher/dpdispatcher/contexts/openapi_context.py", line 10, in <module>
from bohrium import Bohrium
File "/opt/hostedtoolcache/Python/3.12.10/x64/lib/python3.12/site-packages/bohrium/__init__.py", line 1, in <module>
from ._client import Bohrium, AsyncBohrium
File "/opt/hostedtoolcache/Python/3.12.10/x64/lib/python3.12/site-packages/bohrium/_client.py", line 6, in <module>
from httpx import URL, Client, Response, Timeout
ModuleNotFoundError: No module named 'httpx'
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "/home/runner/work/dpdispatcher/dpdispatcher/tests/test_run_submission.py", line 128, in test_failed_submission
machine = Machine.load_from_dict(self.machine_dict)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/dpdispatcher/dpdispatcher/dpdispatcher/machine.py", line 150, in load_from_dict
context = BaseContext.load_from_dict(machine_dict)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/dpdispatcher/dpdispatcher/dpdispatcher/base_context.py", line [45](https://github.com/deepmodeling/dpdispatcher/actions/runs/15847376562/job/44672576823?pr=520#step:6:46), in load_from_dict
context = context_class.load_from_dict(context_dict)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/dpdispatcher/dpdispatcher/dpdispatcher/contexts/openapi_context.py", line 111, in load_from_dict
bohrium_context = cls(
^^^^
File "/home/runner/work/dpdispatcher/dpdispatcher/dpdispatcher/contexts/openapi_context.py", line 68, in __init__
raise ModuleNotFoundError(
ModuleNotFoundError: bohriumsdk not installed. Install dpdispatcher with `pip install dpdispatcher[bohrium]`
Summary by CodeRabbit
New Features
Bug Fixes
Refactor