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

Transport: feat compress & extract methods #6743

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

khsrali
Copy link
Contributor

@khsrali khsrali commented Feb 4, 2025

Introduce two new methods compress & extract for transport plugins.

This is essential for the upcoming developments on stashing features. More details described on issue #6740

@khsrali khsrali requested a review from agoscinski February 4, 2025 16:10
@khsrali khsrali linked an issue Feb 4, 2025 that may be closed by this pull request
Copy link

codecov bot commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 92.15686% with 8 lines in your changes missing coverage. Please review.

Project coverage is 78.16%. Comparing base (d2fbf21) to head (c0900c6).

Files with missing lines Patch % Lines
src/aiida/transports/transport.py 91.38% 5 Missing ⚠️
src/aiida/transports/plugins/ssh_async.py 93.19% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6743      +/-   ##
==========================================
+ Coverage   78.13%   78.16%   +0.04%     
==========================================
  Files         564      564              
  Lines       42526    42627     +101     
==========================================
+ Hits        33222    33316      +94     
- Misses       9304     9311       +7     

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

src/aiida/transports/plugins/ssh_async.py Outdated Show resolved Hide resolved
tests/transports/test_all_plugins.py Outdated Show resolved Hide resolved
tests/transports/test_all_plugins.py Outdated Show resolved Hide resolved
tests/transports/test_all_plugins.py Outdated Show resolved Hide resolved
src/aiida/transports/transport.py Show resolved Hide resolved
src/aiida/transports/transport.py Outdated Show resolved Hide resolved
tests/transports/test_all_plugins.py Outdated Show resolved Hide resolved
tests/transports/test_all_plugins.py Outdated Show resolved Hide resolved
tests/transports/test_all_plugins.py Outdated Show resolved Hide resolved
)

# if creating the tar file fails
# I do this by monkey patching transport.exec_command_wait_async
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain by this monkey patching results in an error? There are multiple reasons for an OSError

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 raises, because the exit code is mocked to 1 instead of 0.
Ok I added match= for all with pytest.raises(OSError, cases.

@khsrali
Copy link
Contributor Author

khsrali commented Feb 7, 2025

@agoscinski thanks a lot, your review is applied, now.
Please have look again, if you have time. Thanks.

@khsrali khsrali requested a review from agoscinski February 7, 2025 10:01
@khsrali khsrali force-pushed the transport/compress-zip branch from 28300c7 to a8bac9a Compare February 7, 2025 10:33
def test_compress_glob(
custom_transport, create_file_hierarchy, file_hierarchy, format, tmp_path_remote, tmp_path_local
):
"""Test the glob functionality of the compress method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Test the glob functionality of the compress method.
"""Test the glob functionality of the compress method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

],
)
def test_compress_glob(
custom_transport, create_file_hierarchy, file_hierarchy, format, tmp_path_remote, tmp_path_local
Copy link
Contributor

Choose a reason for hiding this comment

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

if not too much work, could you add typehints for readability? Its hard enough to find the fixtures if one does not know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I added type hints. Note we can only put the return type of those fixtures there.

def test_compress_glob(
    custom_transport: Transport,
    create_file_hierarchy: callable,
    file_hierarchy: dict,
    format: str,
    tmp_path_remote: Path,
    tmp_path_local: Path
) -> None:

Comment on lines 189 to 191
It's similar to ``test_compress_basic`` but specifies a glob pattern for the source.
Since we had to solve several issues with the glob pattern, I found it appropriate to test it separately.
By isolation we can debug faster in case of a future problem."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
It's similar to ``test_compress_basic`` but specifies a glob pattern for the source.
Since we had to solve several issues with the glob pattern, I found it appropriate to test it separately.
By isolation we can debug faster in case of a future problem."""
It is similar to :func:`test_compress_basic` but specifies a glob pattern for the remote source allowing us to test
the resolving mechanism separately.
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

tests/transports/test_all_plugins.py Outdated Show resolved Hide resolved
tests/transports/test_all_plugins.py Outdated Show resolved Hide resolved
Comment on lines 37 to 40
"""Mock the remote tmp path using tmp_path_factory to create folder start with prefix 'remote'
remote prefix was used for clarification,
if in future we decided to run these tests against an actual remote machine instead of `localhost`.
"""
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 reason is not that we want to run it in the future on remote. I think it is to have two separate tmp path's, one to imitate local, and one to imitate remote. See for example test_put_get_empty_string_tree that uses both fixtures.

Suggested change
"""Mock the remote tmp path using tmp_path_factory to create folder start with prefix 'remote'
remote prefix was used for clarification,
if in future we decided to run these tests against an actual remote machine instead of `localhost`.
"""
"""Provides a tmp path for mocking local and remote computer directory environment.
Local and remote directories must be kept separate to ensure proper functionality testing.
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah sure. But also in case you ever want to run this against a remote server, it's easier this way.
Since ideally one would only need to update that tmp_path_remote to point to somewhere on remote.

@khsrali
Copy link
Contributor Author

khsrali commented Feb 11, 2025

RTD failing is unrelated to this PR:

WARNING: failed to reach any of the inventories with the following issues:
intersphinx inventory 'https://pandas.pydata.org/docs/objects.inv' not fetchable due to <class 'requests.exceptions.HTTPError'>: 522 Server Error:  for url: https://pandas.pydata.org/docs/objects.inv

Update: their server was down, now it works again.

@khsrali khsrali requested a review from agoscinski February 12, 2025 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

transport.compress and transport.extract
2 participants