update: add interruptible execution of loops decorator - #272
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces a synchronous job-polling implementation with an interruptible, decorator-wrapped async variant and updates the notebook call site to use await. Also bumps the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
|
||
|
|
||
| def wait_for_jobs_to_finish(endpoint: JobEndpoints, job_ids: list, poll_interval: int = 10) -> None: | ||
| def _print_jobs_status_table(counts: Counter) -> None: |
Member
There was a problem hiding this comment.
| show_controls: bool = True, | ||
| ): | ||
| """ | ||
| Decorator for single-iteration polling functions. |
| show_controls: bool = True, | ||
| ) -> None: | ||
| """ | ||
| Minimal wrapper. |
| from functools import wraps | ||
| from typing import Any, Awaitable, Callable | ||
|
|
||
| from mat3ra.utils.jupyterlite.environment import ENVIRONMENT, EnvironmentsEnum |
|
|
||
|
|
||
| @dataclass | ||
| class BroadcastChannelAbortController: |
timurbazhirov
approved these changes
Mar 9, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
utils/api.py (1)
63-71: Update docstring to document the return value.The function now returns a
boolindicating whether jobs are still active, but the docstring doesn't document this. This is important for understanding the polling semantics.📝 Suggested docstring update
""" Waits for jobs to finish and prints their statuses. A job is considered finished if it is not in "pre-submission", "submitted", or, "active" status. Args: endpoint (JobEndpoints): Job endpoint object from the Exabyte API Client job_ids (list): list of job IDs to wait for + + Returns: + bool: True if any jobs are still active (polling should continue), False if all jobs have finished. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/api.py` around lines 63 - 71, Update the docstring for wait_for_jobs_to_finish_async to include a Returns section that documents the boolean return value: state that the function returns a bool indicating whether any of the supplied jobs are still active after polling (True if at least one job remains in an active/submitted/pre-submission state, False if all jobs are finished), and mention the polling semantics/timing if relevant; edit the docstring immediately above the function definition for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@other/materials_designer/workflows/band_gap.ipynb`:
- Around line 592-594: The call to wait_for_jobs_to_finish_async is passing an
unsupported keyword argument poll_interval; update the call in the notebook to
call wait_for_jobs_to_finish_async(client.jobs, [job_id]) (remove
poll_interval=POLL_INTERVAL) and remove the now-unused POLL_INTERVAL variable
declaration earlier in the notebook; target the invocation of
wait_for_jobs_to_finish_async and the POLL_INTERVAL definition to make these
changes.
- Line 592: Replace usages of the removed synchronous wait_for_jobs_to_finish
with the new async function wait_for_jobs_to_finish_async from utils.api: update
imports to "from utils.api import wait_for_jobs_to_finish_async" and change all
calls to either "await wait_for_jobs_to_finish_async(...)" inside async
cells/functions or wrap calls in a top-level runner like "import asyncio;
asyncio.run(wait_for_jobs_to_finish_async(...))" in notebook cells that must
remain synchronous; update every notebook that imports or calls
wait_for_jobs_to_finish (search for that symbol) and ensure the call site uses
await or asyncio.run and any surrounding functions are marked async if needed.
In `@pyproject.toml`:
- Line 15: Update the pyproject.toml dependency entry for mat3ra-utils to
include a minimum version that provides the interruptible_polling_loop decorator
(used in utils/api.py); change the existing bare "mat3ra-utils" requirement to a
constrained requirement for the release that added interruptible_polling_loop,
then run a quick install/test to confirm the import in utils/api.py succeeds.
---
Nitpick comments:
In `@utils/api.py`:
- Around line 63-71: Update the docstring for wait_for_jobs_to_finish_async to
include a Returns section that documents the boolean return value: state that
the function returns a bool indicating whether any of the supplied jobs are
still active after polling (True if at least one job remains in an
active/submitted/pre-submission state, False if all jobs are finished), and
mention the polling semantics/timing if relevant; edit the docstring immediately
above the function definition for clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00b05956-1ea1-4325-8aca-14ac2b510f66
📒 Files selected for processing (3)
other/materials_designer/workflows/band_gap.ipynbpyproject.tomlutils/api.py
Comment on lines
+592
to
+594
| "from utils.api import wait_for_jobs_to_finish_async\n", | ||
| "\n", | ||
| "wait_for_jobs_to_finish(client.jobs, [job_id], poll_interval=POLL_INTERVAL)" | ||
| "await wait_for_jobs_to_finish_async(client.jobs, [job_id], poll_interval=POLL_INTERVAL)" |
There was a problem hiding this comment.
TypeError: wait_for_jobs_to_finish_async does not accept poll_interval argument.
The function call passes poll_interval=POLL_INTERVAL, but the new function signature in utils/api.py is:
def wait_for_jobs_to_finish_async(endpoint: JobEndpoints, job_ids: List[str]) -> bool:This will raise TypeError: wait_for_jobs_to_finish_async() got an unexpected keyword argument 'poll_interval' at runtime.
🐛 Proposed fix
- "await wait_for_jobs_to_finish_async(client.jobs, [job_id], poll_interval=POLL_INTERVAL)"
+ "await wait_for_jobs_to_finish_async(client.jobs, [job_id])"Additionally, the POLL_INTERVAL variable defined on line 63 becomes unused and can be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@other/materials_designer/workflows/band_gap.ipynb` around lines 592 - 594,
The call to wait_for_jobs_to_finish_async is passing an unsupported keyword
argument poll_interval; update the call in the notebook to call
wait_for_jobs_to_finish_async(client.jobs, [job_id]) (remove
poll_interval=POLL_INTERVAL) and remove the now-unused POLL_INTERVAL variable
declaration earlier in the notebook; target the invocation of
wait_for_jobs_to_finish_async and the POLL_INTERVAL definition to make these
changes.
| "outputs": [], | ||
| "source": [ | ||
| "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
# Search for other usages of the old wait_for_jobs_to_finish function
rg -n "wait_for_jobs_to_finish[^_]" --type-add 'notebook:*.ipynb' -t notebook -t pyRepository: Exabyte-io/api-examples
Length of output: 2314
🏁 Script executed:
# Locate utils.api module
fd -e py -e ipynb "utils" | grep -i api | head -20Repository: Exabyte-io/api-examples
Length of output: 49
🏁 Script executed:
# Find files containing wait_for_jobs_to_finish definition (not just imports)
rg -n "def wait_for_jobs_to_finish" --type py
# Also search for the utils directory structure
git ls-files | grep -E "(utils|api)" | head -20Repository: Exabyte-io/api-examples
Length of output: 651
🏁 Script executed:
# Check the utils/api.py file for function definitions
cat -n utils/api.py | head -100Repository: Exabyte-io/api-examples
Length of output: 4301
Update all notebooks importing the removed wait_for_jobs_to_finish function.
The synchronous function wait_for_jobs_to_finish has been removed from utils/api.py and replaced with wait_for_jobs_to_finish_async. However, at least 8 notebooks still import or call the old function and will fail with ImportError:
other/jarvis/run_job_using_material_from_jarvis_db.ipynbexamples/reproducing_publications/band_structure_for_interface_bilayer_twisted_molybdenum_disulfide.ipynbexamples/job/run-simulations-and-extract-properties.ipynbexamples/job/ml-train-model-predict-properties.ipynbexamples/job/get-file-from-job.ipynbexamples/workflow/qe_scf_calculation.ipynbexamples/reproducing_publications/band_gaps_for_interface_bilayer_twisted_molybdenum_disulfide.ipynb
Update all affected notebooks to use wait_for_jobs_to_finish_async before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@other/materials_designer/workflows/band_gap.ipynb` at line 592, Replace
usages of the removed synchronous wait_for_jobs_to_finish with the new async
function wait_for_jobs_to_finish_async from utils.api: update imports to "from
utils.api import wait_for_jobs_to_finish_async" and change all calls to either
"await wait_for_jobs_to_finish_async(...)" inside async cells/functions or wrap
calls in a top-level runner like "import asyncio;
asyncio.run(wait_for_jobs_to_finish_async(...))" in notebook cells that must
remain synchronous; update every notebook that imports or calls
wait_for_jobs_to_finish (search for that symbol) and ensure the call site uses
await or asyncio.run and any surrounding functions are marked async if needed.
This was referenced Apr 4, 2026
This was referenced Apr 14, 2026
Merged
This was referenced May 14, 2026
Closed
Merged
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
Chores
Refactor