datalad / datalad-ria

Adds functionality for RIA stores to DataLad
http://datalad.org
Other
0 stars 1 forks source link

Reimplementation of the ORA special remote #6

Closed adswa closed 1 year ago

adswa commented 1 year ago

I suspect that a useful first step towards the re-implementation of RIA functionality is a new and improved ORA remote. This PR wants to do this. At the moment, its minimal first commits are pushed to invite comments and collaboration.

No design decisions have been made other than to base it on datalad-next's SpecialRemote base class.

adswa commented 1 year ago

Considerations about the legacy ORA remote: The plan is to strip RIA functionality completely from datalad core. But if we want to have a fall-back "legacy" mode to the previous implementation, the entire ria code in datalad core needs to move into this extension, too, in some sort of legacy name space.

EDIT: After a chat with @mih it became clear that this extension should not contain legacy RIA code. The most appropriate place for legacy RIA code is datalad-deprecated, and this extension can import from there if needed.

adswa commented 1 year ago

The name space is a bit funny. The archivist special remote implementation had the advantage that it came with a new name - so there was no conflict, and the legacy implementation of datalad-archives was able to exist and be found and used as it was called. But this reimplementation attempt currently goes under the same name ora. And the fact that we keep the legacy around causes a name conflict. Maybe we should rename the new special remote implementation to forgo this issue. e.g., NORA for "new ora" or something like this...

adswa commented 1 year ago

I just saw that there is not only the OraRemote in datalad/distributed/ora_remote.py, but that there also is DeprecatedRIARemote in datalad/customremotes/ria_remote.py . I'm just recording the conscious decision to entirely ignore this class.

adswa commented 1 year ago

Recording three more thoughts that arose in a chat with @mih about details of the reimplementation and the procedure to achieve them:

Make use of a number of datalad-next features

Consider breaking up protocols into separate special remotes

The ORA code is /really/ long and quite difficult to understand, and one aspect that makes it so long is an IO abstraction layer that implements dedicated IO for HTTP(S), file, and ssh protocols in the same special remote. A potential alternative to consider are three stand-alone special remotes: https://github.com/datalad/datalad-ria/issues/30. sameas configurations would be able to connect them.

Better testing is an important prerequisite

codecov-commenter commented 1 year ago

Codecov Report

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

Comparison is base (182e376) 96.87% compared to head (82b14cd) 92.09%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6 +/- ## ========================================== - Coverage 96.87% 92.09% -4.78% ========================================== Files 16 17 +1 Lines 352 405 +53 Branches 0 32 +32 ========================================== + Hits 341 373 +32 - Misses 11 15 +4 - Partials 0 17 +17 ``` | [Files](https://app.codecov.io/gh/datalad/datalad-ria/pull/6?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad) | Coverage Δ | | |---|---|---| | [datalad\_ria/tests/test\_ora.py](https://app.codecov.io/gh/datalad/datalad-ria/pull/6?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad#diff-ZGF0YWxhZF9yaWEvdGVzdHMvdGVzdF9vcmEucHk=) | `100.00% <100.00%> (ø)` | | | [datalad\_ria/ora\_remote.py](https://app.codecov.io/gh/datalad/datalad-ria/pull/6?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad#diff-ZGF0YWxhZF9yaWEvb3JhX3JlbW90ZS5weQ==) | `75.00% <75.00%> (ø)` | | ... and [8 files with indirect coverage changes](https://app.codecov.io/gh/datalad/datalad-ria/pull/6/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datalad)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mih commented 1 year ago

I think this is good to go. It is not a full reimplementation, but a meaningful step, and mostly tests and checks should be added now. This can be done in smaller PRs.