feature/SOF-7961: add scoped material bridge helpers - #355
Open
VsevolodX wants to merge 6 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe PR adds material synchronization for notebook namespaces, Pyodide host-bridge transmission, guarded notebook imports, a Materials Designer REPL preamble, new notebook profiles, and package reinstall tracking. Notebook runtime integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NotebookNamespace
participant sync_materials
participant send_data
participant send_data_pyodide
participant HostBridge
NotebookNamespace->>sync_materials: provide globals
sync_materials->>send_data: send serialized material entities
send_data->>send_data_pyodide: forward payload in Pyodide
send_data_pyodide->>HostBridge: send through bridge or display fallback
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
VsevolodX
force-pushed
the
feature/SOF-7961
branch
from
August 6, 2026 17:44
142f86d to
cf3972c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/py/mat3ra/notebooks_utils/preamble/material.py`:
- Around line 14-20: Update the module-level __all__ definition to contain an
explicit list of known string names instead of expanding _HELPER_NAMES
dynamically, resolving Ruff PLE0604. Preserve the existing noqa: F403 on the
wildcard import.
In `@src/py/mat3ra/notebooks_utils/pyodide/io.py`:
- Around line 49-52: Update the serialization in the io transport flow to call
json.dumps with allow_nan=False, ensuring NaN and infinity payloads are rejected
before either the direct JSON.parse/sendDataToHost bridge or fallback transport
runs. Add a regression test covering non-finite payload values and verify both
transports receive no divergent serialized result.
In `@src/py/mat3ra/notebooks_utils/pyodide/packages/install.py`:
- Around line 114-119: The URL-package exclusion in should_reinstall_package
currently prevents changed wheel URLs from triggering reinstalls. Update this
function to compare URL requirements by distribution name and full URL,
returning true when the requested URL differs from the previously installed URL
while preserving existing version-specifier behavior. Add a regression test
covering the same distribution requested from two different emfs: wheel URLs.
- Around line 191-202: Update the package-state logic around
should_install_packages so an existing requirements_hash is not trusted when
requirements_packages is unset; treat the missing package-state variable as
unstable and force the installation path. Preserve parsed package data only when
the environment variable is present, and add coverage for the existing
requirements_hash/missing requirements_packages case.
🪄 Autofix
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 Plus
Run ID: aad2dbbf-dd8f-4612-87d3-ff593719ee42
📒 Files selected for processing (11)
config.ymlsrc/py/mat3ra/notebooks_utils/core/entity/material/io.pysrc/py/mat3ra/notebooks_utils/io.pysrc/py/mat3ra/notebooks_utils/ipython/io.pysrc/py/mat3ra/notebooks_utils/preamble/__init__.pysrc/py/mat3ra/notebooks_utils/preamble/material.pysrc/py/mat3ra/notebooks_utils/pyodide/io.pysrc/py/mat3ra/notebooks_utils/pyodide/packages/install.pytests/py/unit/core/entity/test_material_io.pytests/py/unit/test_pyodide_io.pytests/py/unit/test_pyodide_packages_install.py
🚧 Files skipped from review as they are similar to previous changes (7)
- src/py/mat3ra/notebooks_utils/preamble/init.py
- src/py/mat3ra/notebooks_utils/ipython/io.py
- src/py/mat3ra/notebooks_utils/io.py
- tests/py/unit/core/entity/test_material_io.py
- tests/py/unit/test_pyodide_io.py
- tests/py/unit/test_pyodide_packages_install.py
- src/py/mat3ra/notebooks_utils/core/entity/material/io.py
Comment on lines
+14
to
+20
| __all__ = [ | ||
| "CoordinatesShapeEnum", | ||
| "Material", | ||
| "NanoparticleShapesEnum", | ||
| "PointDefectTypeEnum", | ||
| *_HELPER_NAMES, | ||
| ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
ruff --version
for config in pyproject.toml ruff.toml .ruff.toml; do
if [ -f "$config" ]; then
rg -n 'PLE0604|ruff|select|ignore' "$config" || true
fi
done
ruff check src/py/mat3ra/notebooks_utils/preamble/material.pyRepository: mat3ra/api-examples
Length of output: 1863
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== pyproject ruff section ==='
sed -n '120,190p' pyproject.toml || true
echo '=== material.py relevant lines ==='
sed -n '1,40p' src/py/mat3ra/notebooks_utils/preamble/material.py
echo '=== ruff check with only PLE0604 enabled ==='
ruff check src/py/mat3ra/notebooks_utils/preamble/material.py --select PLE0604 || trueRepository: mat3ra/api-examples
Length of output: 2149
Use a known string list for __all__.
Current Ruff setup flags __all__ on line 14 with PLE0604 because the starred expansion comes from dynamic helper imports. Either define __all__ with explicit string names, or reassign it after module load; keep the noqa: F403 here since the wildcard import is used.
🧰 Tools
🪛 Ruff (0.16.1)
[error] 14-14: Invalid object in __all__, must contain only strings
(PLE0604)
🤖 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 `@src/py/mat3ra/notebooks_utils/preamble/material.py` around lines 14 - 20,
Update the module-level __all__ definition to contain an explicit list of known
string names instead of expanding _HELPER_NAMES dynamically, resolving Ruff
PLE0604. Preserve the existing noqa: F403 on the wildcard import.
Source: Linters/SAST tools
Comment on lines
+49
to
+52
| serialized_data = json.dumps(payload) | ||
|
|
||
| if JSON is not None and sendDataToHost is not None: | ||
| sendDataToHost(JSON.parse(serialized_data)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
serialized="$(python - <<'PY'
import json
print(json.dumps({"value": float("nan")}))
PY
)"
printf 'Serialized payload: %s\n' "$serialized"
if ! command -v node >/dev/null 2>&1; then
echo "Node.js is required for this verification." >&2
exit 2
fi
if node -e 'JSON.parse(process.argv[1])' "$serialized"; then
echo "Unexpectedly accepted non-standard JSON." >&2
exit 1
else
echo "Confirmed: JavaScript JSON.parse rejects the payload."
fiRepository: mat3ra/api-examples
Length of output: 795
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Target file exists:\n'
ls -l src/py/mat3ra/notebooks_utils/pyodide/io.py
printf '\nRelevant source:\n'
sed -n '1,120p' src/py/mat3ra/notebooks_utils/pyodide/io.py
printf '\nSearch for sendDataToHost and JSON usage:\n'
rg -n "sendDataToHost|JSON\.parse|json\.dumps|allow_nan|nan|NaN|Infinity|Infinity|-Infinity" src/py/mat3ra/notebooks_utils/pyodide -SRepository: mat3ra/api-examples
Length of output: 4693
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Find sendDataToHost definitions/usages in repo:\n'
rg -n "sendDataToHost|window\.sendDataToHost|JSON\.parse|json\.dumps\(.*,\s*allow_nan\s*=\s*False|allow_nan" . -S
printf '\nProbe Python/JS behavior for edge non-finite payloads if Node is available:\n'
python3 - <<'PY'
import json
for value, label in [
(float("nan"), "nan"),
(float("inf"), "inf"),
(float("-inf"), "-inf"),
(complex(1, 2), "complex"),
]:
try:
s = json.dumps({"value": value})
except Exception as exc:
print(label, "python_raises", type(exc).__name__, str(exc))
continue
print(label, "python", s)
PY
node - <<'JS'
for (const s of ['{"value": NaN}', '{"value": Infinity}', '{"value": -Infinity}', '{"value": 1+2j}']) {
try {
JSON.parse(s);
console.log("node accepted", s);
} catch (e) {
console.log("node rejected", s, e.name + ": " + e.message);
}
}
JSRepository: mat3ra/api-examples
Length of output: 1794
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Find sendDataToHost definitions/usages in repo:\n'
rg -n "sendDataToHost|window\.sendDataToHost|JSON\.parse|json\.dumps\(.*,\s*allow_nan\s*=\s*False|allow_nan" . -S
printf '\nProbe Python/JS behavior for edge non-finite payloads if Node is available:\n'
python3 - <<'PY'
import json
for value, label in [
(float("nan"), "nan"),
(float("inf"), "inf"),
(float("-inf"), "-inf"),
(complex(1, 2), "complex"),
]:
try:
s = json.dumps({"value": value})
except Exception as exc:
print(label, "python_raises", type(exc).__name__, str(exc))
continue
print(label, "python", s)
PY
node - <<'JS'
for (const s of ['{"value": NaN}', '{"value": Infinity}', '{"value": -Infinity}', '{"value": 1+2j}']) {
try {
JSON.parse(s);
console.log("node accepted", s);
} catch (e) {
console.log("node rejected", s, e.name + ": " + e.message);
}
}
JSRepository: mat3ra/api-examples
Length of output: 1794
Reject non-finite JSON before both transports.
json.dumps emits NaN and Infinity, but JSON.parse rejects them, so the direct Pyodide bridge can fail. The fallback path may evaluate some of these values successfully, which creates divergent transport behavior. Use json.dumps(payload, allow_nan=False) before transport, and add a regression test for NaN/infinity payloads.
🤖 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 `@src/py/mat3ra/notebooks_utils/pyodide/io.py` around lines 49 - 52, Update the
serialization in the io transport flow to call json.dumps with allow_nan=False,
ensuring NaN and infinity payloads are rejected before either the direct
JSON.parse/sendDataToHost bridge or fallback transport runs. Add a regression
test covering non-finite payload values and verify both transports receive no
divergent serialized result.
Comment on lines
+114
to
+119
| def should_reinstall_package(pkg: str, previous_packages: List[str]) -> bool: | ||
| package_name = get_package_name(pkg) | ||
| if not package_name or not package_has_version_specifier(pkg) or is_url_package(remove_nodeps_prefix(pkg)): | ||
| return False | ||
| previous_spec = next((item for item in previous_packages if get_package_name(item) == package_name), None) | ||
| return previous_spec is not None and previous_spec != pkg |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate target file =="
git ls-files | rg 'install\.py$|config\.yml|pyodide|notebooks_utils'
echo "== target outlines =="
ast-grep outline src/py/mat3ra/notebooks_utils/pyodide/packages/install.py --view expanded || true
echo "== relevant source lines =="
cat -n src/py/mat3ra/notebooks_utils/pyodide/packages/install.py | sed -n '1,180p'
echo "== search for should_reinstall_package/install_package_pyodide/usages =="
rg -n "should_reinstall_package|install_package_pyodide|package_has_version_specifier|is_url_package|remove_nodeps_prefix|emfs:/drive/packages|micropip\.install" src/py/mat3ra/notebooks_utils/pyodide/packages/install.py . || trueRepository: mat3ra/api-examples
Length of output: 20999
🌐 Web query:
Micropip Pyodide install URL wheel reinstall=False compatible incompatible installed package documentation
💡 Result:
In Pyodide's micropip library, the install function includes a reinstall parameter that controls how the system handles packages that are already present in the environment [1][2]. The behavior is as follows: reinstall=False (Default): If a requested package is already installed but is incompatible with the version being requested, micropip will raise an error [1][3][4]. reinstall=True: If a requested package is already installed but is incompatible, micropip will uninstall the existing incompatible package and then install the new version [1][2]. Important considerations when using the reinstall parameter: Imported Modules: Micropip does not automatically reload modules that have already been imported into the Python namespace [1][2]. After a successful reinstallation, you must manually reload any modules you have already imported, for example, by using importlib.reload(module) [1][3]. Package Compatibility: Micropip can only install pure Python wheels or wheels specifically built for the wasm32/emscripten platform (Pyodide) [1][5]. If a package is not found in the Pyodide repository, it will attempt to load it from PyPI, provided it meets these wheel requirements [1][2]. Uninstalling: The separate micropip.uninstall function is also available and behaves similarly regarding imported modules—it removes the package from the environment but not from the active namespace [1][2]. It only supports uninstalling packages that were installed using a wheel file (i.e., those with distribution metadata) [1][6].
Citations:
- 1: https://micropip.pyodide.org/en/stable/project/api.html
- 2: https://micropip.pyodide.org/_/downloads/en/latest/pdf/
- 3: https://micropip.pyodide.org/en/latest/project/api.html
- 4: ENH Add support for reinstalling packages (take 2) pyodide/micropip#206
- 5: https://micropip.pyodide.org/en/stable/project/usage.html
- 6: https://micropip.pyodide.org/en/0.10.0/project/api.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== install_packages_pyodide implementation =="
cat -n src/py/mat3ra/notebooks_utils/pyodide/packages/install.py | sed -n '180,260p'
echo "== package install tests =="
cat -n tests/py/unit/test_pyodide_packages_install.py | sed -n '1,140p'
echo "== deterministic guard behavior for URL wheels =="
python3 - <<'PY'
import re
from typing import List, Union
NODEPS_PREFIX = "nodeps:"
URL_PREFIXES = ("http://", "https://", "emfs:/")
VERSION_SPECIFIERS = ("==", ">=", "<=", "!=", "~=", ">", "<")
def get_package_name(pkg: str) -> Union[str, None]:
spec = pkg.replace(NODEPS_PREFIX, "", 1) if pkg.startswith(NODEPS_PREFIX) else pkg
match = re.match(r"^[A-Za-z0-9_.-]+", spec)
return match.group(0) if match else None
def is_url_package(pkg: str) -> bool:
return pkg.startswith(URL_PREFIXES)
def package_has_version_specifier(pkg: str) -> bool:
spec = pkg.replace(NODEPS_PREFIX, "", 1) if pkg.startswith(NODEPS_PREFIX) else pkg
return any(op in spec for op in VERSION_SPECIFIERS)
def should_reinstall_package(pkg: str, previous_packages: List[str]) -> bool:
package_name = get_package_name(pkg)
if not package_name or not package_has_version_specifier(pkg) or is_url_package(pkg):
return False
previous_spec = next((item for item in previous_packages if get_package_name(item) == package_name), None)
return previous_spec is not None and previous_spec != pkg
cases = [
["emfs:/drive/packages/pymatgen-A.whl", ["emfs:/drive/packages/pymatgen-B.whl"]],
["https://example.com/specific.whl", ["https://example.com/changed.whl"]],
["emfs:/drive/packages/pymatgen-A.whl", []],
]
for pkg, previous_packages in cases:
print(pkg, previous_packages, "=>", should_reinstall_package(pkg, previous_packages))
PYRepository: mat3ra/api-examples
Length of output: 2647
Reinstall changed URL wheels.
should_reinstall_package() returns False for every URL request, including emfs:/drive/packages/*.whl entries. When a changed wheel is requested after the same distribution was installed, install_package_pyodide() calls micropip.install() with reinstall=False, and Micropip reports an incompatible-installed-package error instead of replacing the wheel. Compare URL requirements by distribution name and full URL, and reinstall when the URL changes. Add a regression test for the same distribution requested from two different emfs: wheel URLs.
🤖 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 `@src/py/mat3ra/notebooks_utils/pyodide/packages/install.py` around lines 114 -
119, The URL-package exclusion in should_reinstall_package currently prevents
changed wheel URLs from triggering reinstalls. Update this function to compare
URL requirements by distribution name and full URL, returning true when the
requested URL differs from the previously installed URL while preserving
existing version-specifier behavior. Add a regression test covering the same
distribution requested from two different emfs: wheel URLs.
Comment on lines
+191
to
+202
| previous_packages = json.loads(os.environ.get("requirements_packages", "[]")) | ||
| if should_install_packages(previous_hash, requirements_hash): | ||
| for pkg in packages: | ||
| await install_package_pyodide( | ||
| pkg, | ||
| verbose, | ||
| reinstall=should_reinstall_package(pkg, profile_changed), | ||
| reinstall=should_reinstall_package(pkg, previous_packages), | ||
| ) | ||
| if verbose: | ||
| log("Packages installed successfully.", force_verbose=verbose) | ||
| os.environ["requirements_hash"] = requirements_hash | ||
| os.environ["requirements_packages"] = json.dumps(packages) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'src/py/mat3ra/notebooks_utils/pyodide/packages/install.py|requirements_packages|requirements_hash|should_reinstall_package|should_install_packages' || true
echo
echo "install.py outline:"
ast-grep outline src/py/mat3ra/noteboards_utils/pyodide/packages/install.py --view expanded 2>/dev/null || ast-grep outline src/py/mat3ra/notebooks_utils/pyodide/packages/install.py --view expanded || true
echo
echo "install.py relevant lines:"
cat -n src/py/mat3ra/notebooks_utils/pyodide/packages/install.py | sed -n '1,260p'
echo
echo "Search package-state helpers/usages:"
rg -n 'requirements_packages|requirements_hash|should_reinstall_package|should_install_packages|previous_packages' src/py/mat3ra/notebooks_utils/pyodide/packages/install.py src -SRepository: mat3ra/api-examples
Length of output: 12320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
def should_install_packages(previous_hash, requirements_hash):
return previous_hash != requirements_hash
def get_package_name(pkg):
spec = pkg.removeprefix("nodeps:")
import re
match = re.match(r"^[A-Za-z0-9_.-]+", spec)
return match.group(0) if match else None
def package_has_version_specifier(pkg):
spec = pkg.removeprefix("nodeps:")
return any(op in spec for op in ["==", ">=", "<=", "!=", "~=", ">", "<"])
def is_url_package(pkg):
return pkg.startswith(("http://", "https://", "emfs:/"))
def should_reinstall_package(pkg, previous_packages):
package_name = get_package_name(pkg)
if not package_name or not package_has_version_specifier(pkg) or is_url_package(pkg.removeprefix("nodeps:")):
return False
previous_spec = next((item for item in previous_packages if get_package_name(item) == package_name), None)
return previous_spec is not None and previous_spec != pkg
# Original hash-change behavior
current_hash = "current-hash"
previous_packages_json_missing = os.environ.get("requirements_packages", "[]") if False else "[]"
previous_packages_expected_false = json.loads(previous_packages_json_missing)
print("current_should_install_packages_missing_hash=True:", should_install_packages(None, current_hash))
print("current_should_install_packages_matching_empty_packages:", should_install_packages(current_hash, current_hash))
print("should_reinstall_package_with_empty_previous_packages:", should_reinstall_package("numpy==1.0", json.loads("[]"))
def main():
pass
PYRepository: mat3ra/api-examples
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
import os
import re
from typing import List, Union
def should_install_packages(previous_hash: Union[str, None], requirements_hash: str) -> bool:
return previous_hash != requirements_hash
def remove_nodeps_prefix(pkg: str) -> str:
return pkg.replace("nodeps:", "", 1) if pkg.startswith("nodeps:") else pkg
VERSION_SPECIFIERS = ("==", ">=", "<=", "!=", "~=", ">", "<")
def package_has_version_specifier(pkg: str) -> bool:
spec = remove_nodeps_prefix(pkg)
return any(op in spec for op in VERSION_SPECIFIERS)
def is_url_package(pkg: str) -> bool:
return pkg.startswith(("http://", "https://", "emfs:/"))
def get_package_name(pkg: str) -> Union[str, None]:
spec = remove_nodeps_prefix(pkg)
match = re.match(r"^[A-Za-z0-9_.-]+", spec)
return match.group(0) if match else None
def should_reinstall_package(pkg: str, previous_packages: List[str]) -> bool:
package_name = get_package_name(pkg)
if not package_name or not package_has_version_specifier(pkg) or is_url_package(remove_nodeps_prefix(pkg)):
return False
previous_spec = next((item for item in previous_packages if get_package_name(item) == package_name), None)
return previous_spec is not None and previous_spec != pkg
os.environ["requirements_hash"] = "current-hash"
os.environ.pop("requirements_packages", None)
packages = ["numpy==1.0", "scipy==2.0", pandas"]
requirements_hash = os.environ["requirements_hash"]
previous_package_state_missing = json.loads(os.environ.get("requirements_packages", "[]"))
print("previous_packages_missing_state:", previous_package_state_missing)
print("match_hash_skips_reinstall:", should_install_packages(requirements_hash, requirements_hash))
for pkg in packages:
print("should_reinstall_package", pkg, "=>", should_reinstall_package(pkg, previous_package_state_missing))
current_packages = ["numpy==2.0"]
missing_hash_should_install = should_install_packages(None, "current-hash")
print("new_hash_installs_all_with_initial_state:", missing_hash_should_install)
print("reinstall_flags_with_initial_state:", [(pkg, should_reinstall_package(pkg, json.loads("[]"))) for pkg in current_packages])
PYRepository: mat3ra/api-examples
Length of output: 337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
import os
import re
from typing import List, Union
def remove_nodeps_prefix(pkg: str) -> str:
return pkg.replace("nodeps:", "", 1) if pkg.startswith("nodeps:") else pkg
VERSION_SPECIFIERS = ("==", ">=", "<=", "!=", "~=", ">", "<")
def packet_should_reinstall_package(pkg: str, previous_packages: List[str]) -> bool:
def get_package_name(p: str) -> Union[str, None]:
spec = remove_nodeps_prefix(p)
match = re.match(r"^[A-Za-z0-9_.-]+", spec)
return match.group(0) if match else None
def package_has_version_specifier(p: str) -> bool:
spec = remove_nodeps_prefix(p)
return any(op in spec for op in VERSION_SPECIFIERS)
for is_url_package in [lambda p: p.startswith(("http://", "https://", "emfs:/"))]
package_name = get_package_name(pkg)
if not package_name or not package_has_version_specifier(pkg) or is_url_package(remove_nodeps_prefix(pkg)):
return False
previous_spec = next((item for item in previous_packages if get_package_name(item) == package_name), None)
return previous_spec is not None and previous_spec != pkg
os.environ["requirements_hash"] = "current-hash"
os.environ.pop("requirements_packages", None)
packages = ["numpy==1.0", "scipy==2.0"]
requirements_hash = os.environ["requirements_hash"]
previous_package_state_missing = json.loads(os.environ.get("requirements_packages", "[]"))
print("previous_packages_missing_state:", previous_package_state_missing)
print("match_hash_skips_reinstall:", packet_should_reinstall_package(requirements_hash, requirements_hash))
for pkg in packages:
print("should_reinstall_package", pkg, "=>", packet_should_reinstall_package(pkg, previous_package_state_missing))
PYRepository: mat3ra/api-examples
Length of output: 384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
import os
import re
from typing import List, Union
def remove_nodeps_prefix(pkg: str) -> str:
return pkg.replace("nodeps:", "", 1) if pkg.startswith("nodeps:") else pkg
VERSION_SPECIFIERS = ("==", ">=", "<=", "!=", "~=", ">", "<")
def get_package_name(pkg: str) -> Union[str, None]:
spec = remove_nodeps_prefix(pkg)
match = re.match(r"^[A-Za-z0-9_.-]+", spec)
return match.group(0) if match else None
def package_has_version_specifier(pkg: str) -> bool:
spec = remove_nodeps_prefix(pkg)
return any(op in spec for op in VERSION_SPECIFIERS)
def is_url_package(pkg: str) -> bool:
return pkg.startswith(("http://", "https://", "emfs:/"))
def should_reinstall_package(pkg: str, previous_packages: List[str]) -> bool:
package_name = get_package_name(pkg)
if not package_name or not package_has_version_specifier(pkg) or is_url_package(remove_nodeps_prefix(pkg)):
return False
previous_spec = next((item for item in previous_packages if get_package_name(item) == package_name), None)
return previous_spec is not None and previous_spec != pkg
os.environ["requirements_hash"] = "current-hash"
os.environ.pop("requirements_packages", None)
requirements_hash = os.environ["requirements_hash"]
previous_package_state_missing = json.loads(os.environ.get("requirements_packages", "[]"))
print("previous_packages_missing_state:", previous_package_state_missing)
print("same_hash_skips_block:", previous_package_state_missing == [])
for pkg in ["numpy==1.0", "scipy==2.0"]:
print("should_reinstall_package", pkg, "=", should_reinstall_package(pkg, previous_package_state_missing))
current_packages = ["numpy==2.0"]
print("new_hash_installs_all_without_initial_state:", should_reinstall_package(current_packages[0], json.loads("[]")))
PYRepository: mat3ra/api-examples
Length of output: 360
Treat missing requirements_packages as an unstable package state.
If requirements_hash exists but requirements_packages is unset, the fallback [] becomes a cache key. Matching the hash skips installation, then on the first requirements change every versioned package looks absent from previous_packages, so should_reinstall_package returns False. Use separate state presence check before trusting requirements_hash, and add coverage for existing requirements_hash without requirements_packages.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 201-201: use jsonify instead of json.dumps for JSON output
Context: json.dumps(packages)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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 `@src/py/mat3ra/notebooks_utils/pyodide/packages/install.py` around lines 191 -
202, Update the package-state logic around should_install_packages so an
existing requirements_hash is not trusted when requirements_packages is unset;
treat the missing package-state variable as unstable and force the installation
path. Preserve parsed package data only when the environment variable is
present, and add coverage for the existing requirements_hash/missing
requirements_packages case.
Restores the pre-cf3972cc behaviour: yaml is imported inside read_config_into_dict, not at module scope, with the micropip-install comment documenting why. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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
Validation
Related
Summary by CodeRabbit
New Features
Bug Fixes