🐛 Replace unarchiving function #6959

GitHK · 2024-12-12T15:20:26Z

What do these changes do?

When a new style dynamic service has to fetch an input port containing a zip archive, it will try to unzip it.
If it's not able to open it (see issue here), the service would fail to start.

To fix the issue the unarchiving method is replaced with 7zip. The function signature was not changed for the purpose of making it compatible with the previously used version.

Related issue/s

Deflate64 zip-files not supported by dynamic-sidecar powered services #2843

How to test

Dev-ops checklist

No ENV changes or I properly updated ENV (read the instruction)

…64-zip

codecov · 2024-12-12T15:32:19Z

Codecov Report

Attention: Patch coverage is 64.06250% with 23 lines in your changes missing coverage. Please review.

Project coverage is 68.13%. Comparing base (95d208b) to head (9d67dd1).

❗ There is a different number of reports uploaded between BASE (95d208b) and HEAD (9d67dd1). Click for more details.

HEAD has 29 uploads less than BASE

Flag BASE (95d208b) HEAD (9d67dd1)

unittests 30 1

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #6959       +/-   ##
===========================================
- Coverage   88.11%   68.13%   -19.99%     
===========================================
  Files        1592      633      -959     
  Lines       62337    30659    -31678     
  Branches     2012      262     -1750     
===========================================
- Hits        54928    20888    -34040     
- Misses       7074     9711     +2637     
+ Partials      335       60      -275

Flag	Coverage Δ
integrationtests	`64.95% <26.56%> (-0.03%)`	⬇️
unittests	`88.88% <64.06%> (+2.54%)`	⬆️

Components	Coverage Δ
api	`∅ <ø> (∅)`
pkg_aws_library	`∅ <ø> (∅)`
pkg_dask_task_models_library	`∅ <ø> (∅)`
pkg_models_library	`∅ <ø> (∅)`
pkg_notifications_library	`∅ <ø> (∅)`
pkg_postgres_database	`∅ <ø> (∅)`
pkg_service_integration	`∅ <ø> (∅)`
pkg_service_library	`∅ <ø> (∅)`
pkg_settings_library	`∅ <ø> (∅)`
pkg_simcore_sdk	`77.37% <ø> (-8.02%)`	⬇️
agent	`∅ <ø> (∅)`
api_server	`∅ <ø> (∅)`
autoscaling	`∅ <ø> (∅)`
catalog	`∅ <ø> (∅)`
clusters_keeper	`∅ <ø> (∅)`
dask_sidecar	`∅ <ø> (∅)`
datcore_adapter	`∅ <ø> (∅)`
director	`∅ <ø> (∅)`
director_v2	`78.81% <ø> (-12.60%)`	⬇️
dynamic_scheduler	`∅ <ø> (∅)`
dynamic_sidecar	`89.82% <64.06%> (+0.06%)`	⬆️
efs_guardian	`∅ <ø> (∅)`
invitations	`∅ <ø> (∅)`
osparc_gateway_server	`∅ <ø> (∅)`
payments	`∅ <ø> (∅)`
resource_usage_tracker	`∅ <ø> (∅)`
storage	`∅ <ø> (∅)`
webclient	`∅ <ø> (∅)`
webserver	`59.60% <ø> (-28.14%)`	⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95d208b...9d67dd1. Read the comment docs.

odeimaiz

👍

sanderegg

What is the idea behing this change?

Also I was wondering:

the dask-sidecar checks the input file mime type. if it is set to zip, then it does not uncompress

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/nodeports.py

…64-zip

sanderegg

Please read my additional comments. I don't undersatnd how you wanna present this error to the user.

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/nodeports.py

packages/service-library/src/servicelib/archiving_utils.py

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/nodeports.py

…64-zip

sonarcloud · 2024-12-16T13:34:56Z

Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarQube Cloud

pcrespov · 2024-12-16T16:57:13Z

scripts/install_7zip.bash

+mkdir -p /tmp/7zip
+cd /tmp/7zip
+
+curl -LO https://www.7-zip.org/a/7z${SEVEN_ZIP_VERSION}-linux-x64.tar.xz


Q: why not to have our own image with this already in place instead of downloading and installing it every time?

pcrespov · 2024-12-16T16:58:26Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/seven_zip_wrapper.py

+
+    file_count = await _get_file_count(zip_path)
+
+    command = f"7z x {zip_path} -o{output_dir} -bb1"


You should be careful not to end up with segmented paths in the CLI ...

Normally subprocess api allows you to pass the command as a list of arguments. Here I read that it has to be a str. So perhaps the way to go is to add quotes around the paths? (double check)

Something like

command = f"7z x \"{zip_path}\" -o \"{output_dir}\" -bb1

and long option names? 'x' '-o' --> '--output' ?
and it is indeed usually better to use the list of strings if possible

pcrespov · 2024-12-16T17:10:39Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/seven_zip_wrapper.py

+    assert process.stderr  # nosec
+
+    async with progress_bar.sub_progress(
+        steps=file_count, description=IDStr("...")


Not the first time I see using IDStr for descriptions ...
NOTE that:

Calling IDStr("...") is equivalent to calling str("..."). Therefore in this context: it basically does nothing. To perform a validation you need to create a TypeAdapter(IDStgr).validate_python("...")` etc

If you want to annotate a field . Then note that IDStr is intended for annotation of String IDentifiers (e.g. a prj_123456) and not descriptions! There are other constraints for e.g. ShortTruncatedStr or LongTruncatedStr

@pcrespov I think this is a flaw in the ProgressBar class. I guess I did it wrong at the time. but indeed I would just remove that IDStr from there it makes no sense. But probably should go in a separate PR, maybe an issue for this would be nice.

pcrespov · 2024-12-16T17:15:31Z

services/dynamic-sidecar/tests/unit/test_modules_seven_zip_wrapper.py

+
+
+@pytest.fixture
+def to_archive_dir(tmpdir: LocalPath) -> Path:


This is like a deja vu from to year one's reviews :-)

We use everywhere the tmp_path version of this fixture which produces pathlib.Path instead of tmpdir which produces a custom version of pytest LocalPath

Please read https://docs.pytest.org/en/6.2.x/tmpdir.html#the-tmp-path-fixture and use tmp_path: Path instead of tmpdir: LocalPath

pcrespov · 2024-12-16T17:20:35Z

services/dynamic-sidecar/tests/unit/test_modules_seven_zip_wrapper.py

+    try:
+        subprocess.check_output(["7z", "--help"])  # noqa: S607, S603
+    except Exception:  # pylint: disable=broad-except
+        pytest.skip("7z is not installed")


I suggest to add in the dynamic-sidecar Makefile some recipe to install this. Perhaps add it in the skip message or simply make a reference to ./ci/github/helpers/install_7zip.bash

sanderegg

So in the end we replace the zip library just for unzipping correct?
Why not replacing the zip library for unzipping completely instead?
How are they comparable performance wise?

sanderegg · 2024-12-17T07:27:14Z

.github/workflows/ci-testing-deploy.yml

@@ -1304,6 +1304,8 @@ jobs:
          cache-dependency-glob: "**/dynamic-sidecar/requirements/ci.txt"
      - name: show system version
        run: ./ci/helpers/show_system_versions.bash
+      - name: install 7zip
+        run: sudo ./ci/github/helpers/install_7zip.bash


why is this done here and not in the dynamic-sidecar.bash script?

sanderegg · 2024-12-17T07:28:31Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/core/errors.py

+
+
+class SevenZipError(BaseDynamicSidecarError):
+    msg_template = "Could not finish command: '{command}'\nReason: {command_result}"


Suggested change

msg_template = "Could not finish command: '{command}'\nReason: {command_result}"

msg_template = "Could not complete 7zip command: '{command}'\nReason: {command_result}"

sanderegg · 2024-12-17T07:29:47Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/nodeports.py

@@ -36,6 +36,7 @@

 from ..core.settings import ApplicationSettings, get_settings
 from ..modules.notifications import PortNotifier
+from .seven_zip_wrapper import unarchive_zip_to


would it not make sense to put that one also in servicelib?
Also if you ensure compatible interfaces, that would even call for a common interface?

sanderegg · 2024-12-17T07:30:29Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/nodeports.py

@@ -95,7 +96,7 @@ async def upload_outputs(  # pylint:disable=too-many-statements  # noqa: PLR0915
    port_notifier: PortNotifier,
 ) -> None:
    # pylint: disable=too-many-branches
-    logger.debug("uploading data to simcore...")
+    _logger.debug("uploading data to simcore...")


for this kind of logs, I guess you could use the log decorator and spare 1 line of code

sanderegg · 2024-12-17T07:33:53Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/nodeports.py

+async def _move_file_to_input_port(
+    final_path: Path, downloaded_file: Path, dest_folder: PrunableFolder
+) -> None:


this function named _move_file_to_input_port takes arguments that do not have any port in their name. even the function does not check anything port related...
actually I don't even really understand what dest_folder is? was that the original folder where the downloaded_file is living?

so this funciton

moves downloaded_file into final_path

then prunes dest_folder exluding final_path
maybe a bit of refactoring is in order? at least with naming the arguments?

sanderegg · 2024-12-17T07:46:18Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/seven_zip_wrapper.py

+            if line_decoded.startswith("- "):  # check file entry
+                await sub_prog.update(1)


ohla.. hope that is tested

sanderegg · 2024-12-17T07:48:54Z

services/dynamic-sidecar/tests/unit/test_modules_seven_zip_wrapper.py

+
+
+@pytest.fixture
+def internal_tools_unarchived_tools(tmpdir: LocalPath) -> Path:


this is a very weird name...

sanderegg · 2024-12-17T07:49:28Z

services/dynamic-sidecar/tests/unit/test_modules_seven_zip_wrapper.py

+def generate_content(
+    to_archive_dir: Path, sub_dirs: int, files_in_subdirs: int
+) -> None:
+    for i in range(sub_dirs):
+        (to_archive_dir / f"s{i}").mkdir(parents=True, exist_ok=True)
+        for k in range(files_in_subdirs):
+            (to_archive_dir / f"s{i}" / f"{k}.txt").write_text("a" * k)
+


don't we have already such functions in /home/sanderegg/dev/github/osparc-simcore/packages/pytest-simcore/src/pytest_simcore/file_extra.py ?

sanderegg · 2024-12-17T07:51:01Z

services/dynamic-sidecar/tests/unit/test_modules_seven_zip_wrapper.py

+        print(f"{entry}")
+
+
+def _strip_folder_from_path(paths: set[Path], *, to_strip: Path) -> set[Path]:


I fail to really see the added value of a function that calls a one liner ;) but ok

sanderegg · 2024-12-17T07:51:48Z

services/dynamic-sidecar/tests/unit/test_modules_seven_zip_wrapper.py

+        pytest.param(5, 4, id="few_items"),
+    ],
+)
+async def test_ensure_same_interface_as_unarchive_dir(


why not just use a common interface and not have to test that? but just run the usual archive/unarchive tests with standard zip and 7zip?

move archive to port instead of failing

97fd911

GitHK self-assigned this Dec 12, 2024

GitHK added the a:dynamic-sidecar dynamic-sidecar service label Dec 12, 2024

Merge remote-tracking branch 'upstream/master' into pr-osparc-deflate…

29533e9

…64-zip

GitHK added bug buggy, it does not work as expected t:maintenance Some planned maintenance work and removed bug buggy, it does not work as expected labels Dec 12, 2024

GitHK marked this pull request as ready for review December 13, 2024 05:47

GitHK requested review from mguidon, giancarloromeo, pcrespov, odeimaiz, matusdrobuliak66, sanderegg and mrnicegyu11 December 13, 2024 05:47

odeimaiz reviewed Dec 13, 2024

View reviewed changes

sanderegg reviewed Dec 13, 2024

View reviewed changes

Andrei Neagu added 3 commits December 13, 2024 10:45

using log context and rename

ca87d0b

Merge remote-tracking branch 'upstream/master' into pr-osparc-deflate…

7f3e41e

…64-zip

refactor to use less generic error UnsupportedArchiveFormat

be58b0c

GitHK requested a review from sanderegg December 13, 2024 10:15

sanderegg reviewed Dec 13, 2024

View reviewed changes

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/nodeports.py Outdated Show resolved Hide resolved

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/nodeports.py Outdated Show resolved Hide resolved

renaming error

53e0728

pcrespov reviewed Dec 13, 2024

View reviewed changes

packages/service-library/src/servicelib/archiving_utils.py Outdated Show resolved Hide resolved

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/nodeports.py Outdated Show resolved Hide resolved

Andrei Neagu added 5 commits December 16, 2024 13:47

replace unarchiver

65b5291

remove unused

e6d118d

remove comment

2a2f917

refactor 7zip

56d7fe1

fixed broken install

9c82bc5

GitHK changed the title ~~🐛 Move archive to port instead of failing~~ 🐛 Replace unarchiving method Dec 16, 2024

GitHK changed the title ~~🐛 Replace unarchiving method~~ 🐛 Replace unarchiving function Dec 16, 2024

Andrei Neagu added 4 commits December 16, 2024 14:14

Merge remote-tracking branch 'upstream/master' into pr-osparc-deflate…

5a4cfb5

…64-zip

refactor

0f315e0

mypy

7ede41a

added extra test

9d67dd1

GitHK requested review from sanderegg and pcrespov December 16, 2024 13:50

pcrespov reviewed Dec 16, 2024

View reviewed changes

pcrespov approved these changes Dec 16, 2024

View reviewed changes

sanderegg reviewed Dec 17, 2024

View reviewed changes

giancarloromeo approved these changes Dec 17, 2024

View reviewed changes

GitHK added this to the Event Horizon milestone Dec 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Replace unarchiving function #6959

🐛 Replace unarchiving function #6959

GitHK commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading

odeimaiz left a comment

sanderegg left a comment

sanderegg left a comment

sonarcloud bot commented Dec 16, 2024

pcrespov Dec 16, 2024

pcrespov Dec 16, 2024

sanderegg Dec 17, 2024

pcrespov Dec 16, 2024

sanderegg Dec 17, 2024

pcrespov Dec 16, 2024

pcrespov Dec 16, 2024

sanderegg left a comment

sanderegg Dec 17, 2024

sanderegg Dec 17, 2024

sanderegg Dec 17, 2024

sanderegg Dec 17, 2024

sanderegg Dec 17, 2024

sanderegg Dec 17, 2024

sanderegg Dec 17, 2024

sanderegg Dec 17, 2024

sanderegg Dec 17, 2024

sanderegg Dec 17, 2024


		file_count = await _get_file_count(zip_path)

		command = f"7z x {zip_path} -o{output_dir} -bb1"



		@pytest.fixture
		def to_archive_dir(tmpdir: LocalPath) -> Path:



		class SevenZipError(BaseDynamicSidecarError):
		msg_template = "Could not finish command: '{command}'\nReason: {command_result}"

	msg_template = "Could not finish command: '{command}'\nReason: {command_result}"
	msg_template = "Could not complete 7zip command: '{command}'\nReason: {command_result}"

		if line_decoded.startswith("- "): # check file entry
		await sub_prog.update(1)



		@pytest.fixture
		def internal_tools_unarchived_tools(tmpdir: LocalPath) -> Path:

		print(f"{entry}")


		def _strip_folder_from_path(paths: set[Path], *, to_strip: Path) -> set[Path]:

🐛 Replace unarchiving function #6959

Are you sure you want to change the base?

🐛 Replace unarchiving function #6959

Conversation

GitHK commented Dec 12, 2024 • edited Loading

What do these changes do?

Related issue/s

How to test

Dev-ops checklist

codecov bot commented Dec 12, 2024 • edited Loading

Codecov Report

odeimaiz left a comment

Choose a reason for hiding this comment

sanderegg left a comment

Choose a reason for hiding this comment

sanderegg left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 16, 2024

Quality Gate failed

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

sanderegg 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

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

GitHK commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading