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 all 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
17 changes: 10 additions & 7 deletions .firecrest-demo-config.json
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
{
"url": "http://localhost:8000/",
"token_uri": "http://localhost:8080/auth/realms/kcrealm/protocol/openid-connect/token",
"client_id": "firecrest-sample",
"client_secret": "b391e177-fa50-4987-beaf-e6d33ca93571",
"machine": "cluster",
"scratch_path": "/home/service-account-firecrest-sample"
{
"url": "",
"token_uri": "",
"client_id": "",
"client_secret": "",
"compute_resource": "",
"temp_directory": "",
"small_file_size_mb": 5.0,
"workdir": "",
"api_version": "1.16.0"
}
14 changes: 7 additions & 7 deletions .github/workflows/server-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,16 @@

name: Server

# note: there are several bugs with docker image of FirecREST
# that failes this test. We skip this test for now, but should be addressed in a seperate PR than #36
on:
push:
branches: [main]
tags:
- 'v*'
branches-ignore:
- '**'
pull_request:
paths-ignore:
- README.md
- CHANGELOG.md
- "docs/**"
branches-ignore:
- '**'


jobs:

Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@ 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
run: pytest -vv --cov=aiida_firecrest --cov-report=xml --cov-report=term

- name: Upload coverage reports to Codecov
uses: codecov/codecov-action@v3
uses: codecov/codecov-action@v4
with:
name: aiida-firecrest-pytests
flags: pytests
Expand Down
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -132,3 +132,6 @@ dmypy.json
.vscode/
.demo-server/
_archive/


.firecrest-config.json
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 Issue #45
30 changes: 30 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,35 @@
# 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 `_dynamic_info_firecrest_version` to fetch which version of FirecREST api is interacting with.
- 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

- The testing utils responsible for mocking the FirecREST server (specifically FirecrestMockServer) have been replaced with utils monkeypatching pyfirecrest. The FirecREST mocking utils introduced a maintenance overhead that is not in the responsibility of this repository. We still continue to support running with a real FirecREST server and plan to continue running the tests with the [demo docker image](https://github.com/eth-cscs/firecrest/tree/master/deploy/demo) offered by CSCS. The docker image has been disabled for the moment due to some problems (see issue #47).


### 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.
126 changes: 70 additions & 56 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,10 @@

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
## Installation

Install via GitHub or PyPI:

Expand Down Expand Up @@ -44,73 +43,59 @@ You can then create a `Computer` in AiiDA:
$ verdi computer setup
Report: enter ? for help.
Report: enter ! to ignore the default and set no value.
Computer label: firecrest-client
Hostname: unused
Description []: My FirecREST client plugin
Computer label: firecrest-client # your choice
Hostname: unused # your choice, irrelevant
Description []: My FirecREST client plugin # your choice
Transport plugin: firecrest
Scheduler plugin: firecrest
Shebang line (first line of each script, starting with #!) [#!/bin/bash]:
Work directory on the computer [/scratch/{username}/aiida/]:
Mpirun command [mpirun -np {tot_num_mpiprocs}]:
Default number of CPUs per machine: 2
Default amount of memory per machine (kB).: 100
Default number of CPUs per machine: 2 # depending on your compute resource
Default amount of memory per machine (kB).: 100 # depending on your compute resource
Escape CLI arguments in double quotes [y/N]:
Success: Computer<3> firecrest-client created
Report: Note: before the computer can be used, it has to be configured with the command:
Report: verdi -p quicksetup computer configure firecrest firecrest-client
Report: verdi -p MYPROFILE computer configure firecrest firecrest-client
```

```console
$ verdi -p quicksetup computer configure firecrest firecrest-client
$ verdi -p MYPROFILE 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
Server URL: https://firecrest.cscs.ch # this for CSCS
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]:
Compute resource (Machine): daint
Temp directory on server: /scratch/something/ # "A temp directory on user's space on the server for creating temporary files (compression, extraction, etc.)"
FirecREST api version [Enter 0 to get this info from server] [0]: 0
Maximum file size for direct transfer (MB) [Enter 0 to get this info from server] [0]: 0
Report: Configuring computer firecrest-client for user [email protected].
Success: firecrest-client successfully configured for [email protected]
```

You can always check your config with
```console
$ verdi computer show firecrest-client
--------------------------- ------------------------------------
Label firecrest-client
PK 3
UUID 48813c55-1b2b-4afc-a1a1-e0d33a5b6868
Description My FirecREST client plugin
Hostname unused
Transport type firecrest
Scheduler type firecrest
Work directory /scratch/{username}/aiida/
Shebang #!/bin/bash
Mpirun command mpirun -np {tot_num_mpiprocs}
Default #procs/machine 2
Default memory (kB)/machine 100
Prepend text
Append text
--------------------------- ------------------------------------
```

See also the [pyfirecrest CLI](https://github.com/eth-cscs/pyfirecrest), for directly interacting with a FirecREST server.

See [tests/test_calculation.py](tests/test_calculation.py) for a working example of how to use the plugin, via the AiiDA API.

### 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:
After this, everything should function normally through AiiDA with no problems.
See [tests/test_calculation.py](tests/test_calculation.py) for a working example of how to use the plugin, via the AiiDA API.

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.
If you encounter any problems/bug, please don't hesitate to open an issue on this repository.

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

3. Handling of the client secret, which should likely not be stored in the database
Calculations are now running successfully, however, there are still issues regarding efficiency, Could be improved:

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.
2. Each transfer request includes 2 seconds of `sleep` time, imposed by pyfirecrest. One can takes use of their `async` client, but with current design of `aiida-core`, the gain will be minimum. (see the [closing comment of issue#94 on pyfirecrest](https://github.com/eth-cscs/pyfirecrest/issues/94) and [PR#6079 on aiida-core ](https://github.com/aiidateam/aiida-core/pull/6079))

## Development
## For developers

```bash
git clone
Expand All @@ -134,27 +119,45 @@ It is recommended to run the tests via [tox](https://tox.readthedocs.io/en/lates
tox
```

By default, the tests are run using a mock FirecREST server, in a temporary folder
(see [aiida_fircrest.utils_test.FirecrestConfig](aiida_firecrest/utils_test.py)).
This allows for quick testing and debugging of the plugin, without needing to connect to a real server,
but is obviously not guaranteed to be fully representative of the real behaviour.
By default, the tests are run using a monkey patched pyfirecrest.
This allows for quick testing and debugging of the plugin, without needing to connect to a real server, but is obviously not guaranteed to be fully representative of the real behaviour.

You can also provide connections details to a real FirecREST server:
To have a guaranteed proof, you may also provide connections details to a real FirecREST server:

```bash
tox -- --firecrest-config=".firecrest-demo-config.json"
```

The format of the `.firecrest-demo-config.json` file is:

If a config file is provided, tox sets up a client environment with the information
in the config file and uses pyfirecrest to communicate with the server.
```plaintext
┌─────────────────┐───►┌─────────────┐───►┌──────────────────┐
│ aiida_firecrest │ │ pyfirecrest │ │ FirecREST server │
└─────────────────┘◄───└─────────────┘◄───└──────────────────┘
```

if a config file is not provided, it monkeypatches pyfirecrest so we never actually communicate with a server.
```plaintext
┌─────────────────┐───►┌─────────────────────────────┐
│ aiida_firecrest │ │ pyfirecrest (monkeypatched) │
└─────────────────┘◄───└─────────────────────────────┘
```

The format of the `.firecrest-demo-config.json` file, for example is like:


```json
{
"url": "https://firecrest.cscs.ch",
"token_uri": "https://auth.cscs.ch/auth/realms/cscs/protocol/openid-connect/token",
{
"url": "https://firecrest-tds.cscs.ch",
"token_uri": "https://auth.cscs.ch/auth/realms/firecrest-clients/protocol/openid-connect/token",
"client_id": "username-client",
"client_secret": "xyz",
"machine": "daint",
"scratch_path": "/scratch/snx3000/username"
"client_secret": "path-to-secret-file",
"compute_resource": "daint",
"temp_directory": "/scratch/snx3000/username/",
"small_file_size_mb": 5.0,
"workdir": "/scratch/snx3000/username/",
"api_version": "1.16.0"
}
```

Expand All @@ -164,17 +167,18 @@ In this mode, if you want to inspect the generated files, after a failure, you c
tox -- --firecrest-config=".firecrest-demo-config.json" --firecrest-no-clean
```

See [firecrest_demo.py](firecrest_demo.py) for how to start up a demo server,
and also [server-tests.yml](.github/workflows/server-tests.yml) for how the tests are run against the demo server on GitHub Actions.
**These tests were successful against [FirecREST v1.16.0](https://github.com/eth-cscs/firecrest/releases/tag/v1.16.0), except those who require to list directories in a symlink directory, which fail due to a bug in FirecREST. [An issue](https://github.com/eth-cscs/firecrest/issues/205) is open on FirecREST repo about this.**

Instead of a real server (which requires an account and credential), tests can also run against a docker image provided by FirecREST. See [firecrest_demo.py](firecrest_demo.py) for how to start up a demo server, and also [server-tests.yml](.github/workflows/server-tests.yml) for how the tests are run against the demo server on GitHub Actions.

If you want to analyse statistics of the API requests made by each test,
<!-- If you want to analyse statistics of the API requests made by each test,
you can use the `--firecrest-requests` option:

```bash
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,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?


### :bug: Fishing Bugs :bug:

First, start with running tests locally with no `config` file given, that would monkeypatch pyfirecrest. These set of test do not guarantee that the whole 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` or `tox`.

If these tests pass and the bug persists, consider providing a `config` file to run the tests on a docker image or directly on a real server. Be aware of versioning, pyfirecrest doesn't check which version of api it's interacting with. (see https://github.com/eth-cscs/pyfirecrest/issues/116)

If the bug persists and test still passes, then most certainly it's a problem of `aiida-firecrest`.
If not, probably the issue is from FirecREST, you might open an issue to [pyfirecrest](https://github.com/eth-cscs/pyfirecrest) or [`FirecREST`](https://github.com/eth-cscs/firecrest).
Loading
Loading