feature/SOF-7894 fix tests#300
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
📝 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
| - name: api_client | ||
| - mat3ra-api-client |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify YAML parsing and inspect the affected block.
python -m pip install --quiet pyyaml
python - <<'PY'
import yaml
from pathlib import Path
p = Path("config.yml")
text = p.read_text()
yaml.safe_load(text)
print("YAML parse: OK")
for i, line in enumerate(text.splitlines(), start=1):
if 74 <= i <= 82:
print(f"{i:4}: {line}")
PYRepository: Exabyte-io/api-examples
Length of output: 380
🏁 Script executed:
# Search for schema definition or documentation about notebook configuration
rg -A 10 -B 5 "packages_common|packages_pyodide" config.yml | head -100Repository: Exabyte-io/api-examples
Length of output: 2842
🏁 Script executed:
# Check if there are other notebook entries with raw list items (no explicit key) similar to api_client
rg "^\s*- name:" config.yml -A 3 | head -80Repository: Exabyte-io/api-examples
Length of output: 862
🏁 Script executed:
# Search for code that loads and parses config.yml
rg -t py "config\.yml|packages_common|packages_pyodide" --type-list | head -5Repository: Exabyte-io/api-examples
Length of output: 457
🏁 Script executed:
# Look for manifest loading or config parsing logic
fd -e py -e js -e ts | xargs rg -l "config\.(yml|yaml)" | head -10Repository: Exabyte-io/api-examples
Length of output: 83
🏁 Script executed:
# Search for manifest or config loader
rg "def.*manifest|class.*Manifest|packages_common|packages_pyodide" --type py -B 2 -A 5 | head -100Repository: Exabyte-io/api-examples
Length of output: 49
🏁 Script executed:
cat wheel_server.pyRepository: Exabyte-io/api-examples
Length of output: 2482
Fix malformed api_client notebook entry—missing required packages_common key.
The api_client notebook entry is missing its package key, making it incompatible with the manifest schema. All other notebook entries (e.g., api_examples, torch, mace) explicitly declare package types via packages_common: or packages_pyodide:. This malformed structure will cause manifest loaders to fail or silently ignore the packages.
Proposed fix
- name: api_client
- - mat3ra-api-client
+ packages_common:
+ - mat3ra-api-client📝 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.
| - name: api_client | |
| - mat3ra-api-client | |
| - name: 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 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.
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
1 participant
Summary by CodeRabbit
Documentation
Chores