Fix/datalad cache #199

2023-03-20T09:54:25Z

fraimondo commented

2023-03-20 09:54:25 +00:00

(Migrated from github.com)

Fix an issue with datalad cache/locks on independent clones (temporary directories)

The main issue is a scalability issue and the cache/locks set in the home folder. At some point, there are way too many processes looking to get the lock, and even with the retries and waiting periods, they just give up and start failing.

Solution: use os.environ["DATALAD_LOCATIONS_LOCKS"] = ... to set an override (so each process has an independent lock/cache) and then datalad.cfg.reload() to force reload the configuration. This is supposed to propagate the override to subproccesses.

Also, this PR allows to use numbers for verbosity levels.

Fix an issue with datalad cache/locks on independent clones (temporary directories) The main issue is a scalability issue and the cache/locks set in the home folder. At some point, there are way too many processes looking to get the lock, and even with the retries and waiting periods, they just give up and start failing. Solution: use os.environ["DATALAD_LOCATIONS_LOCKS"] = ... to set an override (so each process has an independent lock/cache) and then datalad.cfg.reload() to force reload the configuration. This is supposed to propagate the override to subproccesses. Also, this PR allows to use numbers for verbosity levels.

codecov[bot] commented

2023-03-20 10:02:43 +00:00

(Migrated from github.com)

Codecov Report

Merging #199 (64e259b) into main (8e2a0ae) will decrease coverage by 0.30%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##             main     #199      +/-   ##
==========================================
- Coverage   93.78%   93.49%   -0.30%     
==========================================
  Files          80       80              
  Lines        3316     3334      +18     
  Branches      603      605       +2     
==========================================
+ Hits         3110     3117       +7     
- Misses        137      146       +9     
- Partials       69       71       +2

Flag	Coverage Δ
docs	`100.00% <ø> (ø)`
junifer	`93.48% <55.55%> (-0.30%)`	⬇️

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

Impacted Files	Coverage Δ
junifer/api/cli.py	`69.47% <41.17%> (-6.43%)`	⬇️
junifer/datagrabber/datalad_base.py	`90.55% <80.00%> (-1.19%)`	⬇️

## [Codecov](https://codecov.io/gh/juaml/junifer/pull/199?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report > Merging [#199](https://codecov.io/gh/juaml/junifer/pull/199?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (64e259b) into [main](https://codecov.io/gh/juaml/junifer/commit/8e2a0ae3139ec1868e524225569c6044e03705cd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (8e2a0ae) will **decrease** coverage by `0.30%`. > The diff coverage is `55.55%`. [![Impacted file tree graph](https://codecov.io/gh/juaml/junifer/pull/199/graphs/tree.svg?width=650&height=150&src=pr&token=5H21JuZXMw&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml)](https://codecov.io/gh/juaml/junifer/pull/199?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #199 +/- ## ========================================== - Coverage 93.78% 93.49% -0.30% ========================================== Files 80 80 Lines 3316 3334 +18 Branches 603 605 +2 ========================================== + Hits 3110 3117 +7 - Misses 137 146 +9 - Partials 69 71 +2 ``` | Flag | Coverage Δ | | |---|---|---| | docs | `100.00% <ø> (ø)` | | | junifer | `93.48% <55.55%> (-0.30%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/juaml/junifer/pull/199?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/api/cli.py](https://codecov.io/gh/juaml/junifer/pull/199?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvY2xpLnB5) | `69.47% <41.17%> (-6.43%)` | :arrow_down: | | [junifer/datagrabber/datalad\_base.py](https://codecov.io/gh/juaml/junifer/pull/199?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9kYXRhbGFkX2Jhc2UucHk=) | `90.55% <80.00%> (-1.19%)` | :arrow_down: |

github-actions[bot] commented

2023-03-20 10:08:00 +00:00

(Migrated from github.com)

PR Preview Action v1.3.0
Preview removed because the pull request was closed.
2023-03-20 13:09 UTC

[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.3.0 :---: Preview removed because the pull request was closed. 2023-03-20 13:09 UTC

synchon (Migrated from github.com) requested changes

2023-03-20 12:04:20 +00:00

junifer/api/cli.py

					
				@ -74,6 +74,45 @@ def _parse_elements(element: str, config: Dict) -> Union[List, None]:

				    return elements

Missing return type annotation.

We can also not have the else here as we have early returns for the previous cases.

We can also not have the `else` here as we have early returns for the previous cases.

I wouldn't go with a pass as it might have unforeseen cases. Better to log it or warn?

I wouldn't go with a `pass` as it might have unforeseen cases. Better to log it or warn?

junifer/datagrabber/tests/test_datalad_base.py

					
				@ -160,6 +160,49 @@ def test_datalad_clone_cleanup(

				    assert len(list(datadir.glob("*"))) == 0

				def test_datalad_clone_create_cleanup(concrete_datagrabber: Type) -> None:

Maybe have the type as the base class for datalad stuff?

fraimondo (Migrated from github.com) reviewed

2023-03-20 12:07:42 +00:00

junifer/api/cli.py

					
				@ -74,6 +74,45 @@ def _parse_elements(element: str, config: Dict) -> Union[List, None]:

				    return elements

It's just an invalid string value that is not a valid option and can't be casted to int. We need to continue and then raise the BadParameter. There's no reason to log/warn here. It's a dead-end.

synchon (Migrated from github.com) reviewed

2023-03-20 12:10:07 +00:00

junifer/api/cli.py

					
				@ -74,6 +74,45 @@ def _parse_elements(element: str, config: Dict) -> Union[List, None]:

				    return elements

I would put the return in an else block then and put a comment so that we know why it's like that.

I would put the `return` in an `else` block then and put a comment so that we know why it's like that.

fraimondo (Migrated from github.com) reviewed

2023-03-20 12:14:04 +00:00

junifer/api/cli.py

					
				@ -74,6 +74,45 @@ def _parse_elements(element: str, config: Dict) -> Union[List, None]:

				    return elements

I'll add the comment, but I do not like the else block to contain just the return as it complicates reading.

I'll add the comment, but I do not like the `else` block to contain just the return as it complicates reading.

synchon (Migrated from github.com) reviewed

2023-03-20 12:17:54 +00:00

junifer/api/cli.py

					
				@ -74,6 +74,45 @@ def _parse_elements(element: str, config: Dict) -> Union[List, None]:

				    return elements

Fair enough. It's how Python docs explain one to write it which makes sense to me.

fraimondo (Migrated from github.com) reviewed

2023-03-20 12:18:48 +00:00

junifer/datagrabber/tests/test_datalad_base.py

					
				@ -160,6 +160,49 @@ def test_datalad_clone_cleanup(

				    assert len(list(datadir.glob("*"))) == 0

				def test_datalad_clone_create_cleanup(concrete_datagrabber: Type) -> None:

I don't get it. This is a copy/paste from the other tests

synchon (Migrated from github.com) reviewed

2023-03-20 12:20:06 +00:00

junifer/datagrabber/tests/test_datalad_base.py

					
				@ -160,6 +160,49 @@ def test_datalad_clone_cleanup(

				    assert len(list(datadir.glob("*"))) == 0

				def test_datalad_clone_create_cleanup(concrete_datagrabber: Type) -> None: