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 15 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
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.5.0"
- "aiida-core~=2.5.1.post0"
khsrali marked this conversation as resolved.
Show resolved Hide resolved
37 changes: 26 additions & 11 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

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.4.0](https://github.com/eth-cscs/firecrest/releases/tag/v2.4.0).

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

Expand Down Expand Up @@ -65,11 +65,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/
Copy link
Member

Choose a reason for hiding this comment

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

Is the something folder provide by CSCS? If it is the folder of user's scratch folder it should be more specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a folder that user should allocate in his space for temporary transported files.
I'll add a comment to clarify that.

Report: Configuring computer firecrest-client for user [email protected].
Success: firecrest-client successfully configured for [email protected]
```
Expand Down Expand Up @@ -100,15 +101,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 efficency, 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 it leaves it to hand of PyFirecREST.

khsrali marked this conversation as resolved.
Show resolved Hide resolved
## Development

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

### Testing

There are two types of tests: mocking the PyFirecREST or the FirecREST server.
While the former 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.
khsrali marked this conversation as resolved.
Show resolved Hide resolved
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 +179,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 +200,13 @@ 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 usefull to quickly check if `aiida-firecrest` is behaving as it's expected to do. To run just simply use `pytest`.
khsrali marked this conversation as resolved.
Show resolved Hide resolved


If these tests, pass and still you have trouble in real deploymeny that means your installed version of pyfirecrest is behaving differently from what `aiida-firecrest` expects in `MockFirecrest` in `tests/tests_mocking_pyfirecrest/conftest.py`.
khsrali marked this conversation as resolved.
Show resolved Hide resolved
In order to solve that, first spot what is different and then solve or raise to `pyfirecrest` if relevant.
khsrali marked this conversation as resolved.
Show resolved Hide resolved
Loading
Loading