-
Notifications
You must be signed in to change notification settings - Fork 0
Prepare SpectrumPY for pip packaging #156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
SpectrumPY-Flight/src/spectrumpy_flight/spectrum_launcher.py
Lines 590 to 593 in b2564a2
| def _convert_packet_file(self, path: Path) -> Optional[Path]: | |
| output_dir = REPO_ROOT / "HDF5" | |
| output_dir.mkdir(parents=True, exist_ok=True) | |
| produced_path = output_dir / f"{path.name}.h5" |
_convert_packet_file() now runs from the packaged module, but it still creates output_dir = REPO_ROOT / "HDF5". After pip installation that location is inside site-packages and cannot be created or written by normal users, so converting a packet downloaded via the launcher will fail even before subprocess.run is invoked. Use the new cache directory (e.g. ~/.spectrumpy_flight/HDF5) or another user‑writable location instead of the package directory.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def run_impactbook() -> Path: | ||
| from ImpactBook import ImpactBook | ||
| from .ImpactBook import ImpactBook | ||
|
|
||
| ImpactBook(channel_names, trcdir=str(directory), ExperimentName=experiment_name) | ||
| return (REPO_ROOT / "HDF5" / f"{experiment_name}.h5").resolve() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid writing ImpactBook output into package directory
When running the GUI from an installed wheel, REPO_ROOT points to the package inside site-packages, which is typically read‑only for normal users. run_impactbook() writes results to REPO_ROOT / "HDF5" / <experiment>.h5, so a pip‑installed copy will raise PermissionError before any processing completes. The output directory should be moved to a user‑writable cache (as done for API downloads) rather than the package installation folder.
Useful? React with 👍 / 👎.
| package_root = resources.files("spectrumpy_flight") | ||
|
|
||
| for default in ("HDF5", "Data"): | ||
| resource = package_root.joinpath(default) | ||
| if resource.exists(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace unsupported importlib.resources exists check
The new _collect_olivine_inputs() uses resources.files("spectrumpy_flight").joinpath(default) and then calls resource.exists(). Traversable objects returned by importlib.resources do not implement exists, so the test will raise an AttributeError before any assertions run. Use resource.is_dir()/is_file() or as_file instead of exists to keep the test runnable.
Useful? React with 👍 / 👎.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_6902449889348320ad0b5875b57eb4e1