feature/SOF-7894 Update: load selected packages#300
Closed
VsevolodX wants to merge 6 commits into
Closed
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates package config and wheel pointers, adds ChangesExamples and Config Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
🤖 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 `@examples/workflow/qe_scf_calculation.ipynb`:
- Line 146: The imported function wait_for_jobs_to_finish_async is implemented
as a synchronous def in utils/api.py but is awaited where it's used; fix by
making the implementation truly asynchronous (change to async def
wait_for_jobs_to_finish_async and await any internal async calls/promises) OR
stop treating it as async by importing and calling the synchronous variant
(e.g., use wait_for_jobs_to_finish without await) and update the import line
accordingly so usage and implementation match; ensure any callers of
wait_for_jobs_to_finish_async are updated to await only if the function is
converted to async.
🪄 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: 7e5ab536-c9ca-4b3d-904b-fddbe10ddf24
📒 Files selected for processing (3)
examples/workflow/qe_scf_calculation.ipynbother/experiments/jupyterlite/uploads/C(001)-Ni(111)-Interface.jsonother/materials_designer/uploads/C(001)-Ni(111)-Interface.json
| "source": [ | ||
| "from utils.settings import ENDPOINT_ARGS, ACCOUNT_ID\n", | ||
| "from utils.api import wait_for_jobs_to_finish\n", | ||
| "from utils.api import wait_for_jobs_to_finish_async\n", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify wait_for_jobs_to_finish_async exists in utils.api module
# Check if the async function is defined in utils/api.py
rg -n "def wait_for_jobs_to_finish_async" --type=py
# Also check for any related async job waiting functions
rg -n "async def.*wait.*job" --type=py -A 3Repository: Exabyte-io/api-examples
Length of output: 170
🏁 Script executed:
# Get the implementation of wait_for_jobs_to_finish_async to see if it's actually async
sed -n '66,120p' utils/api.pyRepository: Exabyte-io/api-examples
Length of output: 2412
🏁 Script executed:
# Get a specific line range around line 340 in the notebook to see how the function is called
rg -n "wait_for_jobs_to_finish" examples/workflow/qe_scf_calculation.ipynb -B 2 -A 2Repository: Exabyte-io/api-examples
Length of output: 496
Fix the async function implementation or usage mismatch.
The function wait_for_jobs_to_finish_async is imported and used with await at line 340, but its implementation at utils/api.py:66 is a regular synchronous function (defined with def, not async def). Awaiting a non-async function will cause a runtime TypeError.
Either:
- Convert the function to truly async by adding the
asynckeyword and usingawaitfor its internal operations, or - Remove the
awaitfrom line 340 and update the import to use the synchronouswait_for_jobs_to_finishfunction if it exists.
🤖 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 `@examples/workflow/qe_scf_calculation.ipynb` at line 146, The imported
function wait_for_jobs_to_finish_async is implemented as a synchronous def in
utils/api.py but is awaited where it's used; fix by making the implementation
truly asynchronous (change to async def wait_for_jobs_to_finish_async and await
any internal async calls/promises) OR stop treating it as async by importing and
calling the synchronous variant (e.g., use wait_for_jobs_to_finish without
await) and update the import line accordingly so usage and implementation match;
ensure any callers of wait_for_jobs_to_finish_async are updated to await only if
the function is converted to async.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@config.yml`:
- Around line 77-78: The api_client notebook entry is malformed because it lacks
the required packages key; update the api_client entry to include the
appropriate package key (e.g., packages_common:) and place mat3ra-api-client
under that list so the entry mirrors others like api_examples/torch/mace;
specifically, locate the api_client block and add a packages_common: field whose
value is a list containing "mat3ra-api-client" to restore valid manifest schema.
🪄 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: 104f9415-684d-4ce2-a3d8-18cf0d8cb42b
📒 Files selected for processing (3)
config.ymlexamples/material/create_material.ipynbother/materials_designer/uploads/C(001)-Ni(111)-Interface.json
✅ Files skipped from review due to trivial changes (1)
- other/materials_designer/uploads/C(001)-Ni(111)-Interface.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
config.yml (1)
75-76:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
api_cliententry is still missing thepackages_common:key.The
mat3ra-api-clientlist item on Line 76 has no parent key, leaving it structurally orphaned. The fix proposed in the previous review was not applied. Every other notebook entry with packages declares an explicitpackages_common:orpackages_pyodide:key before its list items.Additionally, the AI summary claims
packages_commonis configured here, but the current code does not reflect that — the entry remains malformed.🛠️ Proposed fix
- name: api_client - - mat3ra-api-client + packages_common: + - mat3ra-api-client🤖 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 `@config.yml` around lines 75 - 76, The config has an orphaned list item "mat3ra-api-client" under the "api_client" entry; add the missing parent key by inserting a "packages_common:" (or the appropriate packages_* key consistent with other notebook entries) directly beneath "api_client" and before the "mat3ra-api-client" list item so that "api_client" contains a properly nested packages_common: list (ensure indentation matches other entries), i.e., update the "api_client" block to include the packages_common key and move "mat3ra-api-client" under it.
🤖 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.
Duplicate comments:
In `@config.yml`:
- Around line 75-76: The config has an orphaned list item "mat3ra-api-client"
under the "api_client" entry; add the missing parent key by inserting a
"packages_common:" (or the appropriate packages_* key consistent with other
notebook entries) directly beneath "api_client" and before the
"mat3ra-api-client" list item so that "api_client" contains a properly nested
packages_common: list (ensure indentation matches other entries), i.e., update
the "api_client" block to include the packages_common key and move
"mat3ra-api-client" under it.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
config.yml (1)
69-70:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix malformed
api_clientnotebook entry (missing package key).Line 70 is a bare list item under the notebook object; this entry should declare
packages_common(or another explicit package key), otherwise manifest consumers can fail or ignore it.Proposed fix
- name: api_client - - mat3ra-api-client + packages_common: + - mat3ra-api-client🤖 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 `@config.yml` around lines 69 - 70, The notebook entry for "api_client" is malformed because it contains a bare list item "mat3ra-api-client" instead of an explicit package key; update the notebook object so the "api_client" entry uses a package key like "packages_common" (or another appropriate key) and set its value to ["mat3ra-api-client"] (i.e., move the bare item under the explicit key). Locate the "api_client" notebook entry and replace the bare list item with a keyed packages field (e.g., packages_common: - mat3ra-api-client) so manifest consumers can correctly parse it.
🤖 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.
Duplicate comments:
In `@config.yml`:
- Around line 69-70: The notebook entry for "api_client" is malformed because it
contains a bare list item "mat3ra-api-client" instead of an explicit package
key; update the notebook object so the "api_client" entry uses a package key
like "packages_common" (or another appropriate key) and set its value to
["mat3ra-api-client"] (i.e., move the bare item under the explicit key). Locate
the "api_client" notebook entry and replace the bare list item with a keyed
packages field (e.g., packages_common: - mat3ra-api-client) so manifest
consumers can correctly parse it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9eb940f-021b-42a7-832e-fff08b075a3c
📒 Files selected for processing (3)
config.ymlpackages/ase-3.25.0-py3-none-any.whlpackages/pymatgen_analysis_defects-2024.4.23-py3-none-any.whl
✅ Files skipped from review due to trivial changes (2)
- packages/pymatgen_analysis_defects-2024.4.23-py3-none-any.whl
- packages/ase-3.25.0-py3-none-any.whl
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
other/materialsproject/api_interoperability_showcase.py (1)
49-51:⚠️ Potential issue | 🟡 MinorSame
get_ipython()availability concern as inimport_materials_from_materialsproject.py.This file has the same issue with
get_ipython()being undefined outside of IPython/Jupyter environments. See the comment inother/materialsproject/import_materials_from_materialsproject.pyfor details and suggested fix.🤖 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 `@other/materialsproject/api_interoperability_showcase.py` around lines 49 - 51, The call to get_ipython().system(...) will fail outside Jupyter because get_ipython() is undefined; update the snippet around get_ipython().system to detect IPython first (e.g., check for 'get_ipython' in globals() or try/except NameError) and fall back to using Python's subprocess (e.g., subprocess.run with shell=False or shell=True and check=True) to run the curl/bash command when not running in an IPython environment; modify the code referencing get_ipython().system so both execution paths run the same command and surface errors consistently.
🧹 Nitpick comments (1)
utils/io.py (1)
218-218: 💤 Low valueConsider using f-string conversion flag.
Per Ruff's suggestion, use
{level!s}instead of{str(level)}for cleaner f-string formatting.♻️ Proposed refactor
HTML( f"<script>{js} " - f"renderjson.set_show_to_level({str(level)}); " + f"renderjson.set_show_to_level({level!s}); " f'renderjson.set_icons("▸","▾"); ' f'document.getElementById("{container_id}").appendChild(renderjson({json_str}))</script>'🤖 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 `@utils/io.py` at line 218, Replace the explicit str() call inside the f-string used to build the JavaScript call to renderjson (the f-string containing "renderjson.set_show_to_level({str(level)});") with the f-string conversion flag {level!s}; locate the f-string in utils.io where the code constructs the JS snippet for renderjson.set_show_to_level and change {str(level)} to {level!s} to follow the Ruff suggestion and produce the same string more cleanly.
🤖 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 `@other/materialsproject/import_materials_from_materialsproject.py`:
- Around line 47-49: The call to get_ipython().system(...) will raise NameError
outside IPython/Jupyter; wrap it with a guard that safely checks for get_ipython
(e.g., try: ipython = get_ipython(); if ipython is not None:
ipython.system('<curl ... env.sh | bash>') except NameError: pass) and, if you
need the same behavior in standalone execution, run the shell command via
subprocess.run or os.system as a fallback; update the code around the
get_ipython() invocation and the ipython.system(...) call accordingly.
In `@utils/io.py`:
- Line 204: The variable named "id" in the assignment id = str(uuid.uuid4())
shadows Python's built-in id(); rename it to a descriptive identifier (for
example container_id or element_id) and update every reference in the
surrounding function/method in utils/io.py to use the new name so behavior is
unchanged and no builtin is shadowed; ensure any return values, dict keys, or
formatted strings that previously used "id" are updated to the new symbol.
---
Duplicate comments:
In `@other/materialsproject/api_interoperability_showcase.py`:
- Around line 49-51: The call to get_ipython().system(...) will fail outside
Jupyter because get_ipython() is undefined; update the snippet around
get_ipython().system to detect IPython first (e.g., check for 'get_ipython' in
globals() or try/except NameError) and fall back to using Python's subprocess
(e.g., subprocess.run with shell=False or shell=True and check=True) to run the
curl/bash command when not running in an IPython environment; modify the code
referencing get_ipython().system so both execution paths run the same command
and surface errors consistently.
---
Nitpick comments:
In `@utils/io.py`:
- Line 218: Replace the explicit str() call inside the f-string used to build
the JavaScript call to renderjson (the f-string containing
"renderjson.set_show_to_level({str(level)});") with the f-string conversion flag
{level!s}; locate the f-string in utils.io where the code constructs the JS
snippet for renderjson.set_show_to_level and change {str(level)} to {level!s} to
follow the Ruff suggestion and produce the same string more cleanly.
🪄 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: 9ceaa146-1777-458a-a34d-90ca6f52d957
📒 Files selected for processing (29)
examples/job/create_and_submit_job.ipynbexamples/job/get-file-from-job.ipynbexamples/job/ml-train-model-predict-properties.ipynbexamples/material/create_material.ipynbexamples/material/get_materials_by_formula.ipynbexamples/material/upload_materials_from_file_poscar.ipynbexamples/reproducing_publications/band_gaps_for_interface_bilayer_twisted_molybdenum_disulfide.ipynbexamples/reproducing_publications/band_structure_for_interface_bilayer_twisted_molybdenum_disulfide.ipynbexamples/system/get_authentication_params.ipynbexamples/workflow/get_workflows.ipynbother/jarvis/run_job_using_material_from_jarvis_db.ipynbother/materials_designer/specific_examples/defect_point_substitution_graphene_simulation.ipynbother/materials_designer/workflows/band_gap.ipynbother/materials_designer/workflows/band_structure.ipynbother/materials_designer/workflows/band_structure_hse.ipynbother/materials_designer/workflows/convergence.ipynbother/materials_designer/workflows/relaxation.ipynbother/materials_designer/workflows/surface_energy.ipynbother/materials_designer/workflows/total_energy.ipynbother/materials_designer/workflows/total_energy_convergence.ipynbother/materials_designer/workflows/total_energy_post_processing.ipynbother/materials_designer/workflows/valence_band_offset.ipynbother/materialsproject/api_interoperability_showcase.ipynbother/materialsproject/api_interoperability_showcase.pyother/materialsproject/import_materials_from_materialsproject.ipynbother/materialsproject/import_materials_from_materialsproject.pyother/webinar/generate-al2o3-slab-structures.pyutils/io.pyutils/visualize.py
✅ Files skipped from review due to trivial changes (9)
- other/materials_designer/workflows/relaxation.ipynb
- other/materials_designer/workflows/total_energy_post_processing.ipynb
- examples/workflow/get_workflows.ipynb
- other/materialsproject/api_interoperability_showcase.ipynb
- other/materials_designer/specific_examples/defect_point_substitution_graphene_simulation.ipynb
- other/materials_designer/workflows/total_energy.ipynb
- examples/reproducing_publications/band_structure_for_interface_bilayer_twisted_molybdenum_disulfide.ipynb
- examples/material/get_materials_by_formula.ipynb
- other/materials_designer/workflows/band_structure.ipynb
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/material/create_material.ipynb
Comment on lines
+47
to
+49
| get_ipython().system( | ||
| 'GIT_BRANCH="dev"; export GIT_BRANCH; curl -s "https://raw.githubusercontent.com/Exabyte-io/api-examples/${GIT_BRANCH}/scripts/env.sh" | bash' | ||
| ) |
There was a problem hiding this comment.
Verify get_ipython() availability in execution context.
The get_ipython() function is only available in IPython/Jupyter environments and will raise a NameError if this script is executed as a standalone Python script outside of IPython.
If this .py file is auto-generated from a notebook and only intended for IPython/Jupyter execution, this is fine. Otherwise, consider guarding the call:
try:
ipython = get_ipython()
if ipython is not None:
ipython.system('GIT_BRANCH="dev"; export GIT_BRANCH; curl -s "https://raw.githubusercontent.com/Exabyte-io/api-examples/${GIT_BRANCH}/scripts/env.sh" | bash')
except NameError:
# Running outside IPython - handle accordingly
passRun the following script to check how this file is intended to be executed:
#!/bin/bash
# Check if there are any execution instructions or documentation for .py files
# Look for README or documentation mentioning .py execution
rg -n -C3 -i "\.py.*execut|run.*\.py|python.*import_materials" --type=md
# Check if there's a shebang and how it's used
fd -e py -x head -n 5 {} \; | rg -A4 "#!/usr/bin/env python"
# Look for similar get_ipython usage patterns
rg -n "get_ipython\(\)" --type=py -C2🧰 Tools
🪛 Ruff (0.15.12)
[error] 47-47: Undefined name get_ipython
(F821)
🤖 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 `@other/materialsproject/import_materials_from_materialsproject.py` around
lines 47 - 49, The call to get_ipython().system(...) will raise NameError
outside IPython/Jupyter; wrap it with a guard that safely checks for get_ipython
(e.g., try: ipython = get_ipython(); if ipython is not None:
ipython.system('<curl ... env.sh | bash>') except NameError: pass) and, if you
need the same behavior in standalone execution, run the shell command via
subprocess.run or os.system as a fallback; update the code around the
get_ipython() invocation and the ipython.system(...) call accordingly.
| else: | ||
| json_str = obj | ||
|
|
||
| id = str(uuid.uuid4()) |
There was a problem hiding this comment.
Rename id to avoid shadowing Python builtin.
The variable name id shadows the built-in id() function. Use a more descriptive name like container_id or element_id.
🔧 Proposed fix
- id = str(uuid.uuid4())
+ container_id = str(uuid.uuid4())
web_dir = os.path.join(os.path.dirname(__file__), "web")
@@ -211,10 +211,10 @@
with open(os.path.join(web_dir, "renderjson.js")) as fp:
js = fp.read()
- display(HTML(f'<style>{css}</style><div id="{id}"></div>'))
+ display(HTML(f'<style>{css}</style><div id="{container_id}"></div>'))
display(
HTML(
f"<script>{js} "
f"renderjson.set_show_to_level({str(level)}); "
f'renderjson.set_icons("▸","▾"); '
- f'document.getElementById("{id}").appendChild(renderjson({json_str}))</script>'
+ f'document.getElementById("{container_id}").appendChild(renderjson({json_str}))</script>'📝 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
| id = str(uuid.uuid4()) | |
| container_id = str(uuid.uuid4()) | |
| web_dir = os.path.join(os.path.dirname(__file__), "web") | |
| with open(os.path.join(web_dir, "renderjson.js")) as fp: | |
| js = fp.read() | |
| display(HTML(f'<style>{css}</style><div id="{container_id}"></div>')) | |
| display( | |
| HTML( | |
| f"<script>{js} " | |
| f"renderjson.set_show_to_level({str(level)}); " | |
| f'renderjson.set_icons("▸","▾"); ' | |
| f'document.getElementById("{container_id}").appendChild(renderjson({json_str}))</script>' | |
| ) | |
| ) |
🧰 Tools
🪛 Ruff (0.15.12)
[error] 204-204: Variable id is shadowing a Python builtin
(A001)
🤖 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 `@utils/io.py` at line 204, The variable named "id" in the assignment id =
str(uuid.uuid4()) shadows Python's built-in id(); rename it to a descriptive
identifier (for example container_id or element_id) and update every reference
in the surrounding function/method in utils/io.py to use the new name so
behavior is unchanged and no builtin is shadowed; ensure any return values, dict
keys, or formatted strings that previously used "id" are updated to the new
symbol.
VsevolodX
changed the title
| @@ -0,0 +1,3 @@ | |||
| version https://git-lfs.github.com/spec/v1 | |||
Member
There was a problem hiding this comment.
We can use pypi or from_pypi to isolate our own from pypi
| packages_pyodide: | ||
| - annotated_types>=0.6.0 | ||
| - ipywidgets | ||
| - typing-extensions==4.14.1 |
Member
There was a problem hiding this comment.
Needs to be installed at the correct version
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.
Summary by CodeRabbit
New Features
Documentation
Chores