Upgrade to be compatible with pyfirecreset v2.6.0 #36

khsrali · 2024-06-27T11:46:29Z

Some TODO's are done
Wherever relevant issues opened and addressed in aiida-core, pyfirecrest, and firecrest
Added new tests. The old tests, although more sophisticated is harder to debug. New tests focuses only on aiida-firecrest by mocking pyfirecrest only, not firecrest server.
~~Current states still depends on un-merged PR#6043, but~~ functional with latest features of aiida v2.5.2

…aiida-core`. - Refactor FirecrestScheduler to handle pagination for retrieving active jobs - dynamic_info() is added to retrieve machine information without user input. - put & get & copy passed manual test. - Added `dereference` option wherever relevant - Added new tests. The old tests, although more suffesticated is harder to debug. New test focuses only on aiida-firecrest by mocking pyfirecrest only, not the server.

…ore dependency

bug fix: iglob() is sending relative path to listdir() compatibility with new version of firecrest: extract() and compress() will take care of submission if needed

agoscinski

Did a first skim through the PR, but I need more time to really understand logic.

.pre-commit-config.yaml

aiida_firecrest/remote_path.py

aiida_firecrest/scheduler.py

aiida_firecrest/transport.py

aiida_firecrest/utils_test.py

tests/tests_mocking_pyfirecrest/test_transport.py

aiida_firecrest/utils_test.py

pyproject.toml

khsrali · 2024-06-28T08:52:34Z

Thanks a lot @agoscinski for your review, I'll wait for more comments :)

README.md

khsrali

Thanks @DropD
please let me know if you have more comments

README.md

agoscinski

There seems some over lap between the files in tests/*py and tests/tests_mocking_pyfirecrest/*py. Can you maybe explain the test files at the top of the files with a docstring more detailed? There seems to be also not a consistent setup between these levels: For example the same function _firecrest_computer is one leve in tests/test_computers.py and one level in tests/tests_mocking_pyfirecrest/conftest.py

.pre-commit-config.yaml

pyproject.toml

aiida_firecrest/scheduler.py

aiida_firecrest/utils_test.py

aiida_firecrest/scheduler.py

pyproject.toml

aiida_firecrest/utils_test.py

Co-authored-by: Rico Haeuselmann <[email protected]>

Co-authored-by: Alexander Goscinski <[email protected]>

unkcpz

Thanks @khsrali . My major concern here is the remove of tests on a mocked firecrest server.
As an alternative you manually reimplement fixtures from Firecrest, which is more flasky. Because any future change make in the server side you need to update your test and can not assure the backward compatibility.

If the docker image of CSCS is not able to use, then maybe push them to fix and report the problem or get the helps?

CHANGELOG.md

unkcpz · 2024-07-17T07:55:25Z

README.md

 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/


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

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

unkcpz · 2024-07-17T08:01:16Z

README.md

+
+
+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`.
+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:


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.

unkcpz · 2024-07-17T08:06:43Z

tests/conftest.py

+    computer = orm.Computer(
+        label="test_computer",
+        description="test computer",
+        hostname="-",
+        workdir=str(_scratch),
+        transport_type="firecrest",
+        scheduler_type="firecrest",
    )
+    computer.set_minimum_job_poll_interval(5)
+    computer.set_default_mpiprocs_per_machine(1)
+    computer.configure(
+        url=" https://URI",
+        token_uri="https://TOKEN_URI",
+        client_id="CLIENT_ID",
+        client_secret=str(_secret_path),
+        client_machine="MACHINE_NAME",
+        small_file_size_mb=1.0,
+        temp_directory=str(_temp_directory),
+    )
+    computer.store()
+    return computer


Is aiida-core test computer fixture you can use (check this one https://github.com/aiidateam/aiida-core/blob/de83e2ce43101ddb8b5aeaa5c3e18d1e5c85590a/src/aiida/manage/tests/pytest_fixtures.py#L548)? At least make this an independent fixture, like f7t_computer.

Yes, we could use that. What is f7t_computer?

unkcpz · 2024-07-17T08:12:39Z

tests/conftest.py

-    record_requests: bool = request.config.getoption("--firecrest-requests")
-    telemetry: RequestTelemetry | None = None
-
-    if config_path is not None:


Therefore I thought to drop this feature for now, will retrieve later if the docker image is maintained again.

If you drop it "for now", it may not come back anymore. If it is maintained and used by CSCS, should we ask them to fix it?

tests/conftest.py

aiida_firecrest/transport.py

unkcpz · 2024-07-17T08:56:57Z

aiida_firecrest/transport.py

+                firecrest_url=self._url,
+                authorization=ClientCredentialsAuth(client_id, secret, token_uri),
+            )
+        except Exception as e:


What is the exact error type when it can not connect. This part has two possible exception types, one is when create Auth, the other is when create client? Can you just raise so it propagate directly from pyfirecrest?

yes, but the whole message {e} is reported, in the next line, (including the actual error type).
I translated everything in one error type for better handling of the plugin later one.
Right now, mainly these errors might raise:
packaging.version.InvalidVersion
requests.exceptions.RequestException

unkcpz · 2024-07-17T15:32:45Z

As discussed with @khsrali offline, I am fine with removing integration test by using firecrest image to mock some real response. As @khsrali pointed out, pyfirecrest did not do it as well. But I'd suggest to still keep it in mind and open an issue to remember to bring it back with the help from CSCS people to provide a robust and easy to deploy docker-compose solution.

agoscinski

As discussed with @khsrali offline, I am fine with removing integration test by using firecrest image to mock some real response. As @khsrali pointed out, pyfirecrest did not do it as well. But I'd suggest to still keep it in mind and open an issue to remember to bring it back with the help from CSCS people to provide a robust and easy to deploy docker-compose solution.

Also talked with @khsrali about it. The suggestion we agreed on would be to keep the tests that are used with the deployed the firecrest docker image, but disable it for the moment, since the docker image is outdated and maintenance of it not our job. So the structure would be.

Testing structure:

┌─────────────────┐─────────►┌─────────────┐───►┌───────────────────────┐
│ aiida_firecrest │ disabled │ pyfirecrest │    │ FirecREST demo docker │
└─────────────┬───┘◄─────────└─────────────┘◄───└───────────────────────┘
         ▲    │                                                             
         │    └──────────────►┌─────────────────┐                           
         │                    │ MockPyFirecrest │                           
         └────────────────────└─────────────────┘                           
        
We implememented two ways test aiida_fircrest. One is by mocking pyfirercrest with a `MockPyFirecrest` class.
The second is deploying the `FirecREST docker <https://github.com/eth-cscs/firecrest/tree/master/deploy/demo>`_
image and communicating with it through pyfirercest. Because the the docker image is out-of-date for the moment,
we disabled these tests until it has been updated.

tests/test_calculation.py

Co-authored-by: Alexander Goscinski <[email protected]>

…erver (#2) * Bringing back the option to run firecrest with a config with a real server --------- Co-authored-by: Ali Khosravi <[email protected]>

tests/conftest.py

.pre-commit-config.yaml

CHANGELOG.md

agoscinski · 2024-07-26T15:26:46Z

tests/conftest.py

+    │ aiida_firecrest │    │ pyfirecrest │    │ FirecREST server │
+    └─────────────────┘◄───└─────────────┘◄───└──────────────────┘
+
+    if `config_path` is not given, it monkeypatches pyfirecrest so we never


Suggested change

if `config_path` is not given, it monkeypatches pyfirecrest so we never

if no config file is not provided, it monkeypatches pyfirecrest so we never

The suggestion is an unclear sentence :)

tests/conftest.py

CI to be added in a separate PR.

Co-authored-by: Alexander Goscinski <[email protected]>

khsrali · 2024-07-30T09:03:08Z

aiida_firecrest/utils_test.py

This file is partially moved to conftest.py
Excepts parts that are related to mock FirectREST server.
We now mock pyfirecrest for simplicity.

codecov · 2024-07-30T09:37:32Z

Codecov Report

Attention: Patch coverage is 79.17808% with 76 lines in your changes missing coverage. Please review.

Project coverage is 79.94%. Comparing base (eb7f751) to head (7254af5).

Files	Patch %	Lines
aiida_firecrest/transport.py	82.25%	55 Missing ⚠️
aiida_firecrest/scheduler.py	61.81%	21 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #36      +/-   ##
==========================================
+ Coverage   76.25%   79.94%   +3.68%     
==========================================
  Files           6        4       -2     
  Lines        1057      678     -379     
==========================================
- Hits          806      542     -264     
+ Misses        251      136     -115

Flag	Coverage Δ
pytests	`79.94% <79.17%> (+3.68%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

agoscinski

Looks good, almost there, only minor comments.

CHANGELOG.md

README.md

agoscinski · 2024-07-30T13:10:52Z

README.md

@@ -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
+
+
+### :bug: Fishing :bug: Bugs :bug:


It is really not a clear title

Suggested change

### :bug: Fishing :bug: Bugs :bug:

### :bug: Reporting bugs

final offer ### :bug: Fishing Bugs :bug:

tests/test_calculation.py

agoscinski · 2024-07-30T13:34:24Z

tests/conftest.py

+        client.mkdir(config.compute_resource, config.workdir, p=True)
+        client.mkdir(config.compute_resource, config.temp_directory, p=True)
+
+        # if record_requests:


Can you write some comment here why you have this commented code?

yes, I had added between line 149-151

agoscinski · 2024-07-30T13:44:49Z

tests/conftest.py

+        self.extract = extract
+        self.copy = copy
+        self.submit = submit
+        self.poll_active = poll_active


This pattern is a bit weird for me. Cant you move the global functions you set here to the class as methods? So for example

class MockFirecrest: def __init__(self, firecrest_url, *args, **kwargs): ... self.whoami = whoami def whoami(...): ...

->

class MockFirecrest: def __init__(self, firecrest_url, *args, **kwargs): ... def whoami(self, ...): ...

If the need to be you can mark them as @staticmethod

Thanks @agoscinski for your good suggestion.
I initially defined it this way just to remember what I have mocked and what I haven't mocked.

But now that tests has been developed, we can do it the right way.

agoscinski · 2024-07-30T13:49:16Z

tests/conftest.py

+
+    if script_remote_path and not Path(script_remote_path).exists():
+        raise FileNotFoundError(f"File {script_remote_path} does not exist")
+    job_id = random.randint(10000, 99999)


Not exactly sure what you mean @khsrali. Because this is for mocking, the job_id does not matter here? Can't you hardcode it if the number does not matter? Using random without a seed is a pattern that looks like opening the door for surprising behaviors or making debugging harder.

tests/test_scheduler.py

agoscinski

I think it is okay for a merge, since a lot of issues depend on FirecREST and waiting until they are solved would block this PR unnecessary long.

Thanks @khsrali for the work! It was probably quite some annoying debugging.

khsrali added 12 commits June 17, 2024 13:42

added tests for transport:get

9e3d30d

new tests moved as a subdirectory of the main tests

56ff1ee

update readme.md

83a4eb8

added test_transport::put*

4052ae7

added test_transport::copy*

a4588f8

added test_scheduler

c9ab8a6

Applied ruff and mypy

f452999

Readme updated

6304a75

_copy_to moved from path.py directly to interface + updated aiida-c…

202e14f

…ore dependency

property _is_open added to skip AttributeError raised by aiida-core

3c3773f

bug fix: when get_job() is retiereving a completed job

96fe286

bug fix: iglob() is sending relative path to listdir() compatibility with new version of firecrest: extract() and compress() will take care of submission if needed

khsrali requested a review from agoscinski June 27, 2024 11:46

agoscinski reviewed Jun 27, 2024

View reviewed changes

khsrali and others added 2 commits July 2, 2024 12:17

Update FirecREST token url

0d45d6b

Update README.md with correct Token URI (#1)

00c4be9

khsrali requested a review from agoscinski July 3, 2024 14:44

added support for glob patterns in

f7519a4

DropD reviewed Jul 11, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

DropD reviewed Jul 11, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

DropD reviewed Jul 11, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

DropD reviewed Jul 11, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

Enforce str for super method has_magic()

8ebe531

khsrali commented Jul 15, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

agoscinski reviewed Jul 15, 2024

View reviewed changes

khsrali and others added 4 commits July 15, 2024 12:08

Apply suggestions from code review

e493fc5

Co-authored-by: Rico Haeuselmann <[email protected]>

Updated tests

c83a893

temporarly turnned off Codecov

b245639

Apply suggestions from code review

be9b52f

khsrali and others added 2 commits July 16, 2024 09:26

Apply suggestions from code review

689ef55

Co-authored-by: Alexander Goscinski <[email protected]>

test_calculation.py retrieved

70450fe

unkcpz requested changes Jul 17, 2024

View reviewed changes

khsrali changed the title ~~Upgrade to be compatible with pyfirecreset v2.5.0~~ Upgrade to be compatible with pyfirecreset v2.6.0 Jul 23, 2024

agoscinski reviewed Jul 26, 2024

View reviewed changes

tests/test_calculation.py Show resolved Hide resolved

khsrali and others added 8 commits July 26, 2024 12:14

Apply suggestions from code review

d2ccb9e

Co-authored-by: Alexander Goscinski <[email protected]>

appied review

64838a6

added a functionality to check the api version

08482c8

updated tests

f866a6f

Bringing back the option to run firecrest with a config with a real s…

6c7101c

…erver (#2) * Bringing back the option to run firecrest with a config with a real server --------- Co-authored-by: Ali Khosravi <[email protected]>

fix tests

364e3a4

fixtests.patch removed

5994285

__future__ added

6323547

agoscinski reviewed Jul 26, 2024

View reviewed changes

khsrali and others added 2 commits July 29, 2024 19:27

Final updates on tests, now functional with server test.

7cc3e97

CI to be added in a separate PR.

Apply suggestions from code review

1b38515

Co-authored-by: Alexander Goscinski <[email protected]>

khsrali commented Jul 30, 2024

View reviewed changes

khsrali added 2 commits July 30, 2024 11:16

Server github action should skip for now

28d481c

check if codecov works

d143add

Readme updated.

6742b6c

agoscinski mentioned this pull request Jul 30, 2024

Reenable tests for demo-server docker image after some investigation #47

Open

agoscinski requested changes Jul 30, 2024

View reviewed changes

khsrali added 2 commits July 30, 2024 17:05

Applied changes from final review

e229561

change from random to itertools

7254af5

agoscinski approved these changes Jul 30, 2024

View reviewed changes

khsrali merged commit 7006d3d into aiidateam:main Jul 30, 2024
7 checks passed

This was referenced Jul 30, 2024

⬆️ Update pyfirecrest requirement from ~=1.4.0 to ~=2.0.0 #34

Closed

Codecov is failing #38

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to be compatible with pyfirecreset v2.6.0 #36

Upgrade to be compatible with pyfirecreset v2.6.0 #36

khsrali commented Jun 27, 2024 •

edited

Loading

agoscinski left a comment

khsrali commented Jun 28, 2024

khsrali left a comment

agoscinski left a comment •

edited

Loading

unkcpz left a comment •

edited

Loading

unkcpz Jul 17, 2024

khsrali Jul 26, 2024

unkcpz Jul 17, 2024

unkcpz Jul 17, 2024

khsrali Jul 26, 2024

unkcpz Jul 17, 2024

unkcpz Jul 17, 2024

khsrali Jul 26, 2024

unkcpz commented Jul 17, 2024

agoscinski left a comment •

edited

Loading

agoscinski Jul 26, 2024

khsrali Jul 30, 2024

khsrali Jul 30, 2024

codecov bot commented Jul 30, 2024 •

edited

Loading

agoscinski left a comment

agoscinski Jul 30, 2024

khsrali Jul 30, 2024

agoscinski Jul 30, 2024

khsrali Jul 30, 2024

agoscinski Jul 30, 2024 •

edited

Loading

agoscinski Jul 30, 2024 •

edited

Loading

khsrali Jul 30, 2024

agoscinski Jul 30, 2024

agoscinski left a comment



		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`.
		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:

	if `config_path` is not given, it monkeypatches pyfirecrest so we never
	if no config file is not provided, it monkeypatches pyfirecrest so we never

Upgrade to be compatible with pyfirecreset v2.6.0 #36

Upgrade to be compatible with pyfirecreset v2.6.0 #36

Conversation

khsrali commented Jun 27, 2024 • edited Loading

agoscinski left a comment

Choose a reason for hiding this comment

khsrali commented Jun 28, 2024

khsrali left a comment

Choose a reason for hiding this comment

agoscinski left a comment • edited Loading

Choose a reason for hiding this comment

unkcpz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz commented Jul 17, 2024

agoscinski left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 30, 2024 • edited Loading

Codecov Report

agoscinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

agoscinski Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski left a comment

Choose a reason for hiding this comment

khsrali commented Jun 27, 2024 •

edited

Loading

agoscinski left a comment •

edited

Loading

unkcpz left a comment •

edited

Loading

agoscinski left a comment •

edited

Loading

codecov bot commented Jul 30, 2024 •

edited

Loading

agoscinski Jul 30, 2024 •

edited

Loading

agoscinski Jul 30, 2024 •

edited

Loading