Feature/SOF-7836 Update: use clusters API#270
Feature/SOF-7836 Update: use clusters API#270VsevolodX wants to merge 2 commits intofeature/SOF-7824from
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR modernizes API client workflows by introducing a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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 (2)
utils/visualize.py (1)
406-408: Filter logic is correct; consider an early return when all results are filtered out.The
"value" in r or "values" in rcheck is the right way to guard displayable entries. One minor usability gap: if every item is stripped by the filter, the function proceeds silently —renderResultsreceives[]with no user-visible feedback.💡 Optional early-return guard
results = [r for r in results if "value" in r or "values" in r] + + if not results: + print("No displayable properties found in results.") + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/visualize.py` around lines 406 - 408, After filtering results with results = [r for r in results if "value" in r or "values" in r], add an early-return guard that detects an empty results list and surfaces a user-visible message instead of passing [] to renderResults; e.g., if not results: call renderResults with a succinct "No displayable results" message (or invoke an existing no-results helper) and return from the current function so downstream code (and renderResults) aren't invoked with an empty list.pyproject.toml (1)
12-19: Both git dependencies are pinned to anonymous commit hashes — prefer named tags.
mat3ra-api-client@141d7dba...andmat3ra-prode@5b2e02dd...are bare SHA pins with no corresponding tag reference. This makes it impossible to tell at a glance what feature or release the pin corresponds to, and makes future bump PRs harder to review. Pin to a release tag (or at minimum add an inline comment with the version/date) once tags are available on those repos.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 12 - 19, The two git dependencies in pyproject.toml are pinned to raw commit SHAs ("mat3ra-api-client @ git+https://github.com/Exabyte-io/api-client.git@141d7dba220f07844e97557dda3ee74087b38c31" and "mat3ra-prode @ git+https://github.com/Exabyte-io/prode.git@5b2e02ddbaf5dd473f0acaf1e1d76ddadc6bb184"); replace each SHA with the corresponding release tag (or branch name) from those repositories (e.g., vX.Y.Z) so the dependency shows a human-readable version, or if tags are not yet available add a short inline comment after the dependency naming the intended version/date and why the SHA is used; update the two entries "mat3ra-api-client" and "mat3ra-prode" accordingly and run your dependency installer to verify resolution.
🤖 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 517-526: The code assumes at least one cluster exists and directly
indexes clusters[0], which raises IndexError when client.clusters.list() returns
an empty list; add a guard that checks the clusters list (e.g., if not clusters
or len(clusters) == 0) before using clusters[0], and handle the empty case by
raising a clear exception or logging an error and aborting/using a fallback
cluster; update the block around the clusters variable and the Compute(...)
instantiation (Compute, cluster, QueueName.D, ppn) to only construct Compute
when a valid cluster is present.
- Around line 103-115: Remove the hardcoded os.environ assignments
(os.environ["API_HOST"], ["API_PORT"], ["API_SECURE"]) and the unused
API_HOST/API_PORT/API_SECURE/API_VERSION variables from the notebook cell;
instead either delete the cell or replace it with a short opt-in snippet or
commented example showing how to set local overrides (e.g., guarded by a boolean
flag like use_local = False or instructions to export env vars externally)
because APIClient.authenticate() reads from environment variables and these
assignments currently force all runs to localhost:3000.
In `@pyproject.toml`:
- Around line 11-12: The comment above the dependency line is incorrect — it
mentions "file URIs" and the three-slash rule, but the dependency
"mat3ra-api-client @
git+https://github.com/Exabyte-io/api-client.git@141d7dba220f07844e97557dda3ee74087b38c31"
is a VCS/git URL; update the comment to accurately describe that this is a git
VCS dependency using a commit SHA via the git+https://...@<commit> format (or
similar VCS URL guidance), and remove any reference to file URI syntax so the
comment correctly reflects the format used by the dependency line.
---
Nitpick comments:
In `@pyproject.toml`:
- Around line 12-19: The two git dependencies in pyproject.toml are pinned to
raw commit SHAs ("mat3ra-api-client @
git+https://github.com/Exabyte-io/api-client.git@141d7dba220f07844e97557dda3ee74087b38c31"
and "mat3ra-prode @
git+https://github.com/Exabyte-io/prode.git@5b2e02ddbaf5dd473f0acaf1e1d76ddadc6bb184");
replace each SHA with the corresponding release tag (or branch name) from those
repositories (e.g., vX.Y.Z) so the dependency shows a human-readable version, or
if tags are not yet available add a short inline comment after the dependency
naming the intended version/date and why the SHA is used; update the two entries
"mat3ra-api-client" and "mat3ra-prode" accordingly and run your dependency
installer to verify resolution.
In `@utils/visualize.py`:
- Around line 406-408: After filtering results with results = [r for r in
results if "value" in r or "values" in r], add an early-return guard that
detects an empty results list and surfaces a user-visible message instead of
passing [] to renderResults; e.g., if not results: call renderResults with a
succinct "No displayable results" message (or invoke an existing no-results
helper) and return from the current function so downstream code (and
renderResults) aren't invoked with an empty list.
| "source": [ | ||
| "import os\n", | ||
| "\n", | ||
| "os.environ[\"API_HOST\"] = \"localhost\"\n", | ||
| "os.environ[\"API_PORT\"] = \"3000\"\n", | ||
| "os.environ[\"API_SECURE\"] = \"false\"\n", | ||
| "\n", | ||
| "\n", | ||
| "API_HOST = \"localhost\"\n", | ||
| "API_PORT = \"3000\"\n", | ||
| "API_SECURE = \"false\"\n", | ||
| "API_VERSION = \"2018-10-01\"" | ||
| ] |
There was a problem hiding this comment.
Critical: hardcoded localhost env vars will break any non-local execution.
These os.environ assignments override the API host, port, and secure settings for the entire process. Any user running this notebook against a real Mat3ra endpoint (staging, prod, or a hosted JupyterLite deployment) will have their API calls silently redirected to localhost:3000, causing connection errors. The four API_* Python variables declared below the env assignments are also unused — APIClient.authenticate() in the next cell reads from the env, not from these local vars.
This looks like a leftover local-dev configuration that should not be committed. The cell should either be removed or converted to clearly opt-in behaviour (e.g., guarded by a flag or replaced with comments showing how to configure a local override).
🛠️ Proposed fix — remove the hard-coded overrides, document as opt-in
-"import os\n",
-"\n",
-"os.environ[\"API_HOST\"] = \"localhost\"\n",
-"os.environ[\"API_PORT\"] = \"3000\"\n",
-"os.environ[\"API_SECURE\"] = \"false\"\n",
-"\n",
-"\n",
-"API_HOST = \"localhost\"\n",
-"API_PORT = \"3000\"\n",
-"API_SECURE = \"false\"\n",
-"API_VERSION = \"2018-10-01\""
+
+"# To target a non-default endpoint, set the environment variables before running:\n",
+"# os.environ[\"API_HOST\"] = \"localhost\"\n",
+"# os.environ[\"API_PORT\"] = \"3000\"\n",
+"# os.environ[\"API_SECURE\"] = \"false\"\n",
+"# The default values come from the mat3ra-api-client configuration."📝 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.
| "source": [ | |
| "import os\n", | |
| "\n", | |
| "os.environ[\"API_HOST\"] = \"localhost\"\n", | |
| "os.environ[\"API_PORT\"] = \"3000\"\n", | |
| "os.environ[\"API_SECURE\"] = \"false\"\n", | |
| "\n", | |
| "\n", | |
| "API_HOST = \"localhost\"\n", | |
| "API_PORT = \"3000\"\n", | |
| "API_SECURE = \"false\"\n", | |
| "API_VERSION = \"2018-10-01\"" | |
| ] | |
| "source": [ | |
| "# To target a non-default endpoint, set the environment variables before running:\n", | |
| "# os.environ[\"API_HOST\"] = \"localhost\"\n", | |
| "# os.environ[\"API_PORT\"] = \"3000\"\n", | |
| "# os.environ[\"API_SECURE\"] = \"false\"\n", | |
| "# The default values come from the mat3ra-api-client configuration." | |
| ] |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 112-112: Found useless expression. Either assign it to a variable or remove it.
(B018)
🤖 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 103 - 115,
Remove the hardcoded os.environ assignments (os.environ["API_HOST"],
["API_PORT"], ["API_SECURE"]) and the unused
API_HOST/API_PORT/API_SECURE/API_VERSION variables from the notebook cell;
instead either delete the cell or replace it with a short opt-in snippet or
commented example showing how to set local overrides (e.g., guarded by a boolean
flag like use_local = False or instructions to export env vars externally)
because APIClient.authenticate() reads from environment variables and these
assignments currently force all runs to localhost:3000.
| "from mat3ra.ide.compute import Compute, QueueName\n", | ||
| "\n", | ||
| "CLUSTER_NAME = get_cluster_name()\n", | ||
| "cluster = clusters[0]\n", | ||
| "\n", | ||
| "compute = client.jobs.build_compute_config(\n", | ||
| " cluster=CLUSTER_NAME\n", | ||
| "compute = Compute(\n", | ||
| " cluster=cluster,\n", | ||
| " queue=QueueName.D,\n", | ||
| " ppn=2\n", | ||
| ")" | ||
| ] |
There was a problem hiding this comment.
clusters[0] raises IndexError when no clusters are available.
client.clusters.list() could return an empty list (no clusters provisioned for the account, API error, etc.), causing an unhandled IndexError on the next line.
🛡️ Proposed guard
cluster = clusters[0]
+if not clusters:
+ raise RuntimeError("No clusters available for this account. Please provision a cluster first.")
+cluster = clusters[0]🤖 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 517 - 526,
The code assumes at least one cluster exists and directly indexes clusters[0],
which raises IndexError when client.clusters.list() returns an empty list; add a
guard that checks the clusters list (e.g., if not clusters or len(clusters) ==
0) before using clusters[0], and handle the empty case by raising a clear
exception or logging an error and aborting/using a fallback cluster; update the
block around the clusters variable and the Compute(...) instantiation (Compute,
cluster, QueueName.D, ppn) to only construct Compute when a valid cluster is
present.
| # Use a valid PEP 508 URL for a local file dependency: file URIs must be absolute and include three slashes | ||
| "mat3ra-api-client @ git+https://github.com/Exabyte-io/api-client.git@141d7dba220f07844e97557dda3ee74087b38c31", |
There was a problem hiding this comment.
Misleading comment — this is a VCS URL, not a file URI.
The comment references "file URIs" and "three slashes" (file:///... syntax), but the actual dependency uses git+https://. The three-slash rule for file URIs is unrelated here. Update the comment to accurately describe the git URL format being used.
📝 Proposed fix
- # Use a valid PEP 508 URL for a local file dependency: file URIs must be absolute and include three slashes
+ # Installed directly from a specific git commit until a PyPI release is available (PEP 508 VCS URL)
"mat3ra-api-client @ git+https://github.com/Exabyte-io/api-client.git@141d7dba220f07844e97557dda3ee74087b38c31",📝 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.
| # Use a valid PEP 508 URL for a local file dependency: file URIs must be absolute and include three slashes | |
| "mat3ra-api-client @ git+https://github.com/Exabyte-io/api-client.git@141d7dba220f07844e97557dda3ee74087b38c31", | |
| # Installed directly from a specific git commit until a PyPI release is available (PEP 508 VCS URL) | |
| "mat3ra-api-client @ git+https://github.com/Exabyte-io/api-client.git@141d7dba220f07844e97557dda3ee74087b38c31", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 11 - 12, The comment above the dependency line
is incorrect — it mentions "file URIs" and the three-slash rule, but the
dependency "mat3ra-api-client @
git+https://github.com/Exabyte-io/api-client.git@141d7dba220f07844e97557dda3ee74087b38c31"
is a VCS/git URL; update the comment to accurately describe that this is a git
VCS dependency using a commit SHA via the git+https://...@<commit> format (or
similar VCS URL guidance), and remove any reference to file URI syntax so the
comment correctly reflects the format used by the dependency line.
| @@ -465,7 +465,8 @@ | |||
| "id": "34", | |||
There was a problem hiding this comment.
2 participants
Summary by CodeRabbit
Bug Fixes
Chores