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

535 singularity #540

Merged
merged 220 commits into from
Feb 8, 2024
Merged

535 singularity #540

merged 220 commits into from
Feb 8, 2024

Conversation

Lukas113
Copy link
Collaborator

closes #535

This PR addresses the issues of #535 . The main work was to make sure, that the disk-caching was not performed in a read-only filesystem, since in Singularity you're not the same user as the creator of the image. Therefore, I refactored FileHandler to have a short-term-memory and long-term-memory disk-cache available. Long-term is mainly for downloaded objects, where short-term-memory is for intermediate data-products, if no file/dir-path was provided in the according API-function. For details, see code & docstring of FileHandler.

In addition, I've seen two instances where I was not happy how it was implemented (in addition with the disk-caching), and saw a good opportunity to refactor the code. The first one was, how DaskHandler was implemented. It had some twisted code with SLURM, where the separation of concerns was not fulfilled. In addition, I've refactored some telescope-function code to be increase the stability & re-usability of the code.

This PR should be merged after #526 is merged, because it's a branch created from 512_sarus.

The testing-setup was, I tested our tests in singularity- sarus & docker-container, and they passed there. This is of course no guarantee that I got everything, however, I've looked through the entire repository & the tests passed which is good enough I think.

lukas.gehrig added 30 commits September 25, 2023 10:56
…fore pushing the image 👠"

This reverts commit a828dfd.
@Lukas113 Lukas113 requested a review from sfiruch January 29, 2024 16:18
Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Attention: 210 lines in your changes are missing coverage. Please review.

Comparison is base (367cf11) 65.72% compared to head (a4a599a) 64.74%.

Files Patch % Lines
karabo/util/dask.py 54.13% 122 Missing ⚠️
karabo/imaging/imager.py 15.38% 33 Missing ⚠️
karabo/util/file_handler.py 83.78% 18 Missing ⚠️
karabo/simulation/interferometer.py 58.62% 12 Missing ⚠️
karabo/simulation/telescope.py 85.71% 10 Missing ⚠️
karabo/simulation/visibility.py 58.33% 5 Missing ⚠️
karabo/data/external_data.py 73.33% 4 Missing ⚠️
karabo/__init__.py 0.00% 2 Missing ⚠️
karabo/imaging/image.py 71.42% 2 Missing ⚠️
karabo/karabo_resource.py 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #540      +/-   ##
==========================================
- Coverage   65.72%   64.74%   -0.99%     
==========================================
  Files          48       48              
  Lines        4537     4686     +149     
==========================================
+ Hits         2982     3034      +52     
- Misses       1555     1652      +97     

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

Copy link
Collaborator

@lmachadopolettivalle lmachadopolettivalle left a comment

Choose a reason for hiding this comment

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

Very well-written code, only a few small comments about documentation and a few methods.

doc/src/examples/example_structure.md Outdated Show resolved Hide resolved
doc/src/examples/example_structure.md Outdated Show resolved Hide resolved
doc/src/examples/example_structure.md Outdated Show resolved Hide resolved
doc/src/examples/example_structure.md Outdated Show resolved Hide resolved
karabo/data/external_data.py Show resolved Hide resolved
karabo/util/file_handler.py Show resolved Hide resolved
karabo/util/dask.py Show resolved Hide resolved
karabo/util/dask.py Show resolved Hide resolved
karabo/util/dask.py Show resolved Hide resolved
karabo/util/dask.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@lmachadopolettivalle lmachadopolettivalle left a comment

Choose a reason for hiding this comment

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

💯

@Lukas113 Lukas113 merged commit 7b52842 into main Feb 8, 2024
3 checks passed
@Lukas113 Lukas113 deleted the 535_singularity branch February 8, 2024 15:39
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.

Karabo works with Singularity Container
2 participants