improved retrievals and adaptions to next pyslk release #48
Conversation
|
@observingClouds Can I prevent somehow that tests are generated automatically for all pyslk functions? Setting up all SlkMock functions properly will take a lot of time. |
|
@neumannd can you point me to some tests? I think I cannot completely follow you. You just need to mock those functions that are actually used within slkspec. |
That's the main issue I have. It is quite time consuming to set up proper mock functions for this purpose. I am aware that this would be the proper proceedure. But, I am lacking time to finished this task in December. Would you merge anyway? |
|
I have to say no here as I believe it's crucial for all code additions to pass the tests to maintain the integrity of the project. |
fix: small fixes to attribute/method calls to pyslk
…riginal functions
Tests/pyslk mock
fix(deps): restrict zarr to <3.0.0 to prevent breaking changes
|
@observingClouds : The tests succeed now. @doguskbilir Updated the mock functions. Do you have any comments on the current code version? Additionally, @doguskbilir is working on setting up a |
|
Amazing! Great work! Yes, please do the |
There was a problem hiding this comment.
Thanks @neumannd and @doguskbilir for this hard work! I really appreciate it. My main comment is about the complexity of _retrieve_item(). Please restructure it so that flake8 does no longer complain about C901. I think with a few extra functions and classes it will become much more readable and help us in the long run to maintain this package better.
| @@ -135,23 +155,686 @@ def name(self) -> str: | |||
| return self._file | |||
| return self._url | |||
|
|
|||
| # flake8: noqa: C901 | |||
There was a problem hiding this comment.
I am sorry to delay this further, but flake8 has correctly identified this >500 line long function to be too complicated. Could you please break this up into several function and use e.g. dataclasses to group some of the variables so that flake8 is happy again, e.g.
@dataclass
class TapeRecallStatus:
success: Set[str] = field(default_factory=set)
active: Set[str] = field(default_factory=set)
failed: Dict[str, str] = field(default_factory=dict)There was a problem hiding this comment.
I'll split it up 👍
There was a problem hiding this comment.
The reason why I did no split this up as completely independent functions -- but used a sub-function (or how they are called) -- is that there are many arguments required and these arguments need to be modified. I think the most reasonable approach here would be to set up a specific class for this ... . Need to think about it.
| """ | ||
| {'SKIPPED': {'SKIPPED_TARGET_EXISTS': ['/arch/bm0146/k204221/iow/INDEX.txt']}, | ||
| 'FILES': {'/arch/bm0146/k204221/iow/INDEX.txt': '/home/k204221/tmp/INDEX.txt'}} | ||
|
|
||
| # dry run | ||
| {'ENVISAGED': {'ENVISAGED': ['/arch/bm0146/k204221/iow/INDEX.txt']}, | ||
| 'FILES': {'/arch/bm0146/k204221/iow/INDEX.txt': '/home/k204221/tmp/abcdef2/INDEX.txt'}} | ||
|
|
||
| # after successful retrieval | ||
| {'ENVISAGED': {'ENVISAGED': []}, 'FILES': {'/arch/bm0146/k204221/iow/INDEX.txt': | ||
| '/home/k204221/tmp/INDEX.txt'}, 'SUCCESS': {'SUCCESS': ['/arch/bm0146/k204221/iow/INDEX.txt']}} | ||
|
|
||
| {'FAILED': {'FAILED_NOT_CACHED': ['/arch/bm0146/k204221/iow/iow_data5_001.tar']}, | ||
| 'FILES': {'/arch/bm0146/k204221/iow/iow_data5_001.tar': '/home/k204221/tmp/iow_data5_001.tar'}} |
There was a problem hiding this comment.
This could go into the docstring of a new function?!
| # +---------------------------------------------------------- | ||
| # | CHECK STATUS OF RUNNING JOBS | ||
| # +---------------------------------------------------------- | ||
| # check if there are jobs running for whole tapes: |
There was a problem hiding this comment.
I am not a fan of these comment, ideally this would become obvious from the function names.
pyslkversion 2.x.y