-
Notifications
You must be signed in to change notification settings - Fork 5
Support firecrest v2 #72
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
Conversation
Co-authored-by: Alexander Goscinski <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #72 +/- ##
==========================================
- Coverage 82.38% 82.04% -0.34%
==========================================
Files 4 4
Lines 806 752 -54
==========================================
- Hits 664 617 -47
+ Misses 142 135 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR updates the Firecrest v2 support and adapts tests, configuration, and scheduler/transport interfaces to the new API changes. Key changes include updating dependencies and imports from pyfirecrest and aiida-core, replacing touch() with write_text("touch") in tests due to empty file handling, and modifying scheduler job query and cancellation methods to align with Firecrest v2.
Reviewed Changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_transport.py | Updated file creation commands and header comments to reference aiida-firecrest. |
| tests/test_scheduler.py | Adjusted job ID testing and comments to reflect updated error handling. |
| tests/test_computer.py & test_calculation.py | Updated assertions and file operations to support Firecrest v2 behavior. |
| tests/conftest.py | Updated Firecrest client import and configuration (billing_account, rm instead of delete). |
| pyproject.toml, .pre-commit-config.yaml | Updated dependency versions and source revisions. |
| aiida_firecrest/utils.py | Modified convert_header_exceptions error conversion and introduced JobNotFoundError. |
| aiida_firecrest/scheduler.py | Adjusted job information extraction, cancellation method, and API field mappings. |
| README.md, GitHub workflows | Updated documentation and CI steps for docker compose with Firecrest v2. |
| .firecrest-demo-config.json | Updated configuration with valid credentials and API details for Firecrest v2. |
Comments suppressed due to low confidence (1)
tests/test_transport.py:13
- [nitpick] It may be helpful to reference this limitation in a common test utilities or documentation section so that future tests follow the same pattern consistently.
FirecREST cannot send over empty files, therefore I do `Path.write_text("touch")` instead of Path.touch() in all tests.
| # TODO: investigate this, if not needed, raise | ||
| pass | ||
| else: | ||
| results = [[]] |
Copilot
AI
Jun 18, 2025
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.
Setting 'results = [[]]' when jobs is empty may lead to runtime errors when iterating over results. Consider initializing 'results' as an empty list instead.
| results = [[]] | |
| results = [] |
| c = converters.get(header) | ||
| if c is not None: | ||
| raise c(data) from exc | ||
| raise c(exc) if callable(c) else c from exc |
Copilot
AI
Jun 18, 2025
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.
[nitpick] The updated error conversion in convert_header_exceptions now conditionally calls the converter; please ensure that all converters provided are callable and that this behavior is well documented for maintainability.
Note to myself:
For some reason to properly launch the docker, I have to rebuild, use this:
sudo docker compose -f 'docker-compose.yml' up -d --build
instead of what's the readme file says in https://github.com/eth-cscs/firecrest-v2
All these issues (in upstream) has to addressed before this PR, could happen:
v1->v2:cpis missing-rfrom the source code, therefore cannot copy directories eth-cscs/pyfirecrest#167v1->v2:cpis silently resolving all symlinks by default an opposite behavior compared to the previous version eth-cscs/pyfirecrest#166v1->v2:UTILITIES_MAX_FILE_SIZEis no longer reported eth-cscs/pyfirecrest#162v1->v2:submitshould also allow for remote paths eth-cscs/pyfirecrest#161v1->v2:whoamiis absent eth-cscs/pyfirecrest#160v1->v2:checksumreturn type has changed eth-cscs/pyfirecrest#159v1->v2:FIRECREST_VERSIONis no longer reported eth-cscs/pyfirecrest#163Transport: bug fix on glob aiida-core#6917What remains to do is to launch the docker on CI, (#66), which is currently functional on local. And disable and dispose the mock tests!
These tests are failing due to the above mentioned issues:
tests/test_calculation.py::test_calculation_file_transfer FAILEDtests/test_transport.py::test_copy[copy] FAILEDtests/test_transport.py::test_copy[copytree] FAILEDtests/test_transport.py::test_copyfile FAILED