Skip to content
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

Upgrade to be compatible with pyfirecreset v2.6.0 #36

Merged
merged 39 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
2113752
- Some TODO's are done, wherever relavant, I implemented changes to …
khsrali May 10, 2024
9e3d30d
added tests for transport:get
khsrali Jun 19, 2024
56ff1ee
new tests moved as a subdirectory of the main tests
khsrali Jun 19, 2024
83a4eb8
update readme.md
khsrali Jun 19, 2024
4052ae7
added test_transport::put*
khsrali Jun 19, 2024
a4588f8
added test_transport::copy*
khsrali Jun 19, 2024
c9ab8a6
added test_scheduler
khsrali Jun 24, 2024
f452999
Applied ruff and mypy
khsrali Jun 24, 2024
6304a75
Readme updated
khsrali Jun 24, 2024
202e14f
`_copy_to` moved from path.py directly to interface + updated aiida-c…
khsrali Jun 25, 2024
3c3773f
property _is_open added to skip AttributeError raised by aiida-core
khsrali Jun 26, 2024
96fe286
bug fix: when get_job() is retiereving a completed job
khsrali Jun 27, 2024
0d45d6b
Update FirecREST token url
khsrali Jul 2, 2024
00c4be9
Update README.md with correct Token URI (#1)
mikibonacci Jul 2, 2024
f7519a4
added support for glob patterns in
khsrali Jul 5, 2024
8ebe531
Enforce str for super method `has_magic()`
khsrali Jul 14, 2024
e493fc5
Apply suggestions from code review
khsrali Jul 15, 2024
c83a893
Updated tests
khsrali Jul 15, 2024
b245639
temporarly turnned off Codecov
khsrali Jul 15, 2024
be9b52f
Apply suggestions from code review
khsrali Jul 15, 2024
f792b45
review applied
khsrali Jul 15, 2024
a238dad
some info added in changelog
khsrali Jul 15, 2024
689ef55
Apply suggestions from code review
khsrali Jul 16, 2024
70450fe
test_calculation.py retrieved
khsrali Jul 16, 2024
d2ccb9e
Apply suggestions from code review
khsrali Jul 26, 2024
64838a6
appied review
khsrali Jul 26, 2024
08482c8
added a functionality to check the api version
khsrali Jul 26, 2024
f866a6f
updated tests
khsrali Jul 26, 2024
6c7101c
Bringing back the option to run firecrest with a config with a real s…
agoscinski Jul 26, 2024
364e3a4
fix tests
khsrali Jul 26, 2024
5994285
fixtests.patch removed
khsrali Jul 26, 2024
6323547
__future__ added
khsrali Jul 26, 2024
7cc3e97
Final updates on tests, now functional with server test.
khsrali Jul 29, 2024
1b38515
Apply suggestions from code review
khsrali Jul 30, 2024
28d481c
Server github action should skip for now
khsrali Jul 30, 2024
d143add
check if codecov works
khsrali Jul 30, 2024
6742b6c
Readme updated.
khsrali Jul 30, 2024
e229561
Applied changes from final review
khsrali Jul 30, 2024
7254af5
change from random to itertools
khsrali Jul 30, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions .firecrest-demo-config.json
khsrali marked this conversation as resolved.
Show resolved Hide resolved

This file was deleted.

96 changes: 0 additions & 96 deletions .github/workflows/server-tests.yml
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is removed, because we don't run the demo version anymore. Unfortunately FirecREST docker image is not being maintained and I was not able to lunch it all functional --the storage service has never worked for me..
If in FirecREST v2, this is resolved, I suggest to retrieve this file along with server-test.

This file was deleted.

21 changes: 11 additions & 10 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,14 @@ jobs:
python -m pip install --upgrade pip
pip install -e .[dev]
- name: Test with pytest
run: pytest -vv --firecrest-requests --cov=aiida_firecrest --cov-report=xml --cov-report=term

- name: Upload coverage reports to Codecov
uses: codecov/codecov-action@v3
with:
name: aiida-firecrest-pytests
flags: pytests
file: ./coverage.xml
fail_ci_if_error: true
token: ${{ secrets.CODECOV_TOKEN }}
run: pytest -vv --cov=aiida_firecrest --cov-report=xml --cov-report=term

# Codecov failing, we need to fix token: https://github.com/aiidateam/aiida-firecrest/issues/38
khsrali marked this conversation as resolved.
Show resolved Hide resolved
# - name: Upload coverage reports to Codecov
# uses: codecov/codecov-action@v3
# with:
# name: aiida-firecrest-pytests
# flags: pytests
# file: ./coverage.xml
# fail_ci_if_error: true
# token: ${{ secrets.CODECOV_TOKEN }}
4 changes: 2 additions & 2 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,5 @@ repos:
additional_dependencies:
- "types-PyYAML"
- "types-requests"
- "pyfirecrest~=1.4.0"
- "aiida-core~=2.4.0"
- "pyfirecrest>=2.6.0"
- "aiida-core>=2.6.0" # please change to 2.6.2 when released
khsrali marked this conversation as resolved.
Show resolved Hide resolved
31 changes: 31 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,36 @@
# Changelog

## v0.2.0 - (not released yet)

### Transport plugin
- Refactor `put` & `get` & `copy` now they mimic behavior `aiida-ssh` transport plugin.
- `put` & `get` & `copy` now support glob patterns.
- Added `dereference` option wherever relevant
- Added `recursive` functionality for `listdir`
- Added `_create_secret_file` to store user secret locally in `~/.firecrest/`
- Added `_validate_temp_directory` to allocate a temporary directory useful for `extract` and `compress` methods on FirecREST server.
- Added `_dynamic_info_direct_size` this is able to get info of direct transfer from the server rather than asking from users. Raise if fails to make a connection.
- Added `_validate_checksum` to check integrity of downloaded/uploaded files.
- Added `_gettreetar` & `_puttreetar` to transfer directories as tar files internally.
- Added `payoff` function to calculate when is gainful to transfer as zip, and when to transfer individually.

### Scheduler plugin
- `get_job` now supports for pagination for retrieving active jobs
- `get_job` is parsing more data than before: `submission_time`, `wallclock_time_seconds`, `start_time`, `time_left`, `nodelist`. see open issues [39](https://github.com/aiidateam/aiida-firecrest/issues/39) & [40](https://github.com/aiidateam/aiida-firecrest/issues/40)
- bug fix: `get_job` won't raise if the job cannot be find (completed/error/etc.)
- `_convert_time` and `_parse_time_string` copied over from `slurm-plugin` see [open issue](https://github.com/aiidateam/aiida-firecrest/issues/42)

### Tests

- Tests has completely replaced with new ones. Previously tests were mocking FirecREST server. Those test were a good practice to ensure that all three (`aiida-firecrest`, FirecREST, and PyFirecREST) work flawlessly.
khsrali marked this conversation as resolved.
Show resolved Hide resolved
The downside was debugging wasn't easy at all. Not always obvious which of the three is causing a bug.
khsrali marked this conversation as resolved.
Show resolved Hide resolved
Because of this, a new set of tests only verify the functionality of `aiida-firecrest` by directly mocking PyFirecREST. Maintaining this set in `tests/` is simpler because we just need to monitor the return values of PyFirecREST​. While maintaining the former is more difficult as you have to keep up with both FirecREST and PyFirecREST.


### Miscellaneous

- class `FcPath` is removed from interface here, as it has [merged](https://github.com/eth-cscs/pyfirecrest/pull/43) into `pyfirecrest`

## v0.1.0 (December 2021)

Initial release.
43 changes: 31 additions & 12 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@

AiiDA Transport/Scheduler plugins for interfacing with [FirecREST](https://products.cscs.ch/firecrest/), via [pyfirecrest](https://github.com/eth-cscs/pyfirecrest).

It is currently tested against [FirecREST v1.13.0](https://github.com/eth-cscs/firecrest/releases/tag/v1.13.0).
It is currently tested against [FirecREST v2.6.0](https://github.com/eth-cscs/pyfirecrest/tree/v2.6.0).

**NOTE:** This plugin is currently dependent on a fork of `aiida-core` from [PR #6043](https://github.com/aiidateam/aiida-core/pull/6043)

## Usage

Expand Down Expand Up @@ -65,11 +64,12 @@ $ verdi -p quicksetup computer configure firecrest firecrest-client
Report: enter ? for help.
Report: enter ! to ignore the default and set no value.
Server URL: https://firecrest.cscs.ch
Token URI: https://auth.cscs.ch/auth/realms/cscs/protocol/openid-connect/token
Token URI: https://auth.cscs.ch/auth/realms/firecrest-clients/protocol/openid-connect/token
Client ID: username-client
Client Secret: xyz
Client Machine: daint
Maximum file size for direct transfer (MB) [5.0]:
Temp directory on server: /scratch/something/ # "A temp directory on user's space on the server for creating temporary files (compression, extraction, etc.)"
Report: Configuring computer firecrest-client for user [email protected].
Success: firecrest-client successfully configured for [email protected]
```
Expand Down Expand Up @@ -100,15 +100,9 @@ See [tests/test_calculation.py](tests/test_calculation.py) for a working example

### Current Issues

Simple calculations are now running successfully [in the tests](tests/test_calculation.py), however, there are still some critical issues, before this could be production ready:
Calculations are now running successfully, however, there are still issues regarding efficiency, Could be improved:

1. Currently uploading via firecrest changes `_aiidasubmit.sh` to `aiidasubmit.sh` 😱 ([see #191](https://github.com/eth-cscs/firecrest/issues/191)), so `metadata.options.submit_script_filename` should be set to this.

2. Handling of large (>5Mb) file uploads/downloads needs to be improved

3. Handling of the client secret, which should likely not be stored in the database

4. Monitoring / management of API request rates could to be improved
1. Monitoring / management of API request rates could to be improved. Currently this is left up to PyFirecREST.

## Development

Expand All @@ -128,6 +122,16 @@ pre-commit run --all-files

### Testing

There are two types of tests: mocking the PyFirecREST or the FirecREST server.
While the latter is a good practice to ensure that all three (`aiida-firecrest`, FirecREST, and PyFirecREST) work flawlessly, debugging may not always be easy because it may not always be obvious which of the three is causing a bug.
Because of this, we have another set of tests that only verify the functionality of `aiida-firecrest` by directly mocking PyFirecREST. Maintaining the second set in `tests/tests_mocking_pyfirecrest/` is simpler because we just need to monitor the return values of PyFirecREST​. While maintaining the former is more difficult as you have to keep up with both FirecREST and PyFirecREST.


#### Mocking FirecREST server

These tests were successful against [FirecREST v1.13.0](https://github.com/eth-cscs/firecrest/releases/tag/v1.13.0).
For newer version please refer to tests Mocking PyFirecREST

It is recommended to run the tests via [tox](https://tox.readthedocs.io/en/latest/).

```bash
Expand Down Expand Up @@ -174,7 +178,7 @@ you can use the `--firecrest-requests` option:
tox -- --firecrest-requests
```

### Notes on using the demo server on MacOS
##### Notes on using the demo server on MacOS

A few issues have been noted when using the demo server on MacOS (non-Mx):

Expand All @@ -195,3 +199,18 @@ although it is of note that you can find these files directly where you your `fi
[codecov-link]: https://codecov.io/gh/aiidateam/aiida-firecrest
[black-badge]: https://img.shields.io/badge/code%20style-black-000000.svg
[black-link]: https://github.com/ambv/black

Copy link
Contributor

@agoscinski agoscinski Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The references (see above)

[codecov-link]: https://codecov.io/gh/aiidateam/aiida-firecrest
[black-badge]: https://img.shields.io/badge/code%20style-black-000000.svg
[black-link]: https://github.com/ambv/black

are typically at the end of the file. Can you also put them there?



#### Mocking PyFirecREST

These set of test do not gurantee that the firecrest protocol is working, but it's very useful to quickly check if `aiida-firecrest` is behaving as it's expected to do. To run just simply use `pytest`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the sentence should be added after tox is introduced. Something like

It is recommended to run the tests via tox. ...
If you want to run the tests in the current environment, run pytest.



If these tests, pass and still you have trouble in real deployment, that means your installed version of pyfirecrest is behaving differently from what `aiida-firecrest` expects in `MockFirecrest` in `tests/tests_mocking_pyfirecrest/conftest.py`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole paragraph is more something like ### Troubleshooting

If there is no version of `aiida-firecrest` available that supports your `pyfirecrest` version and if down/upgrading your `pyfirecrest` to a supported version is not an option, you might try the following:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you mean by "your pyfirecrest"? If I understand correctly, pyfirecrest is a dependency of aiida-firecrest and it is the client so it should all controlled from aiida-firecrest side to pinning down or check the API version is compatible.

- open an issue on the `aiida-firecrest` repository on GitHub to request supporting your version of pyfirecrest
- if you feel up to finding the discrepancy and fixing it within `aiida-firecrest`, open a PR instead
- if you think the problem is a bug in `pyfirecrest`, open an issue there

Either way, make sure to report which version of `aiida-firecrest` and `pyfirecrest` you are using.
Loading
Loading