Patch RIA and ORA code to work on multiple client platforms #669

christian-monch · 2024-04-25T10:51:00Z

This PR addresses the issues with Windows-clients that use RIA-stores. It replaces the classes datalad.distributed.ora_remote.ORARemote and datalad.distributed.create_sibling_ria.CreateSiblingRia with updated versions that should work properly on Linux, OSX, and Windows.

What does it do?

The main difference to the original code is that all path-operations, e.g. to determine the location of ria-layout-version, are performed on URL-paths. Those are represented by instances of PurePosixPath. All subclasses of BaseIO, i.e. LocalIO, SSHRemoteIO, and HTTPRemoteIO, are extended to contain the method url2transport_path. This method converts an URL-path into the correct path for the respective IO abstraction.

Before methods on a subclass of BaseIO that require a path are called, the generic URL-path is converted into the correct path for the IO-class by calling url2transport_path on the respective IO-class.

A number of tests had to be adapted because the patched methods now assert the correct type for the abstract paths, i.e. PurePosixPath. This assertion made several tests fail because they provided OS-paths.

The PR keeps changes to the necessary minimum. That means the source is mostly identical to the original, except for the changes described above.

TODO

Improve test coverage

codecov · 2024-05-07T07:55:32Z

Codecov Report

Attention: Patch coverage is 85.22250% with 176 lines in your changes are missing coverage. Please review.

Project coverage is 92.41%. Comparing base (c5b3620) to head (585b48d).

❗ Current head 585b48d differs from pull request most recent head 2a6772a. Consider uploading reports for the commit 2a6772a to get more accurate results

Files	Patch %	Lines
datalad_next/patches/replace_ora_remote.py	75.42%	76 Missing and 26 partials ⚠️
datalad_next/patches/replace_create_sibling_ria.py	73.89%	33 Missing and 20 partials ⚠️
datalad_next/patches/fix_ria_ora_tests.py	95.32%	12 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #669      +/-   ##
==========================================
- Coverage   93.04%   92.41%   -0.64%     
==========================================
  Files         185      193       +8     
  Lines       12913    14107    +1194     
  Branches     1950     2126     +176     
==========================================
+ Hits        12015    13037    +1022     
- Misses        689      808     +119     
- Partials      209      262      +53

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

mih

Few first, very high-level comments:

The PR adds a number of patches that seem to replace all(?) of the ORARemote implementation -- or it is at least not easy to see which parts are left untouched. No all individual patches are documented properly (top-level docstring explaining the purpose).

This makes it hard to understand why these are individual patches, and not just one -- especially given the strong patch order requirements.

From what I can see, all of the following could be one patch (may still be composed of several files):

add_method_url2transport_path
replace_sshremoteio
ria_utils
replace_ora_remote
fix_ria_ora_tests
replace_create_sibling_ria

Please also at some indication where the, then modified, code was taken from. Further changes might be made in datalad-core, and someone would need to figure out which version to compare to. Grep for taken from in datalad_next/patches.

datalad_next/patches/fix_ria_ora_tests.py

This commit reloads `cls` in `datalad.customremotes.main._main` to allow `cls` to be patched successfully by `load_extensions()`. This is currently used for `ORARemote`-patching. Note: a nicer solution would be to execute: from datalad.support.entrypoints import load_extensions load_extensions() as first commands in the `main`-function of all remote-implementations. But since this would require changes in datalad-core, it was not done here.

This commit add the method `url2transport_path` to the classes `datalad.distributed.ora_remote.SSHRemoteIO`, `datalad.distributed.ora_remote.LocalIO`, and `datalad.distributed.ora_remote.HTTPRemoteIO`. This method is used to convert the path representation that ORA remote will use internally, i.e. `PurePosixPath`, into a path that can be used with the respective IO-abstraction. The necessary modification in ora_remote are contained in a later commit. This change is made mainly to support RIA clients on all platforms, including Windows.

This commit replaces the complete `ORARemote` class with a patched version that does: - consistently use `PurePosixPath` for RIA-directory operations in RIA-manipulation-code, and use the `url2transport_path`-method to convert these paths into IO-abstraction-specific "real" paths - On Windows-clients: use canonified drive-letter encoding internally, and convert them into annex- or git-specific drive letter encodings before calling the respective methods. With these changes an ORARemote will properly work on Windows.

This commit replaces the class `datalad.distributed.create_sibling_ria.CreateSiblingRia`. to adapt its methods to the generic path-handling for RIA-store path manipulation-code. This commit is part of The commit also contains a patch with the neccessary changes to `ria_utils.py`.

This commit adapts tests that use RIA- and ORA-code to use the internal RIA-path representation, i.e. `PurePosixPath`. That has become necessary because the patched RIA code asserts that path-arguments are instances of `PurePosixPath`.

This commit activates the test `datalad_next.patches.tests.test_ria.test_ria_ssh_roundtrip` on Windows. With the previous commits in this branch, the test should now pass.

This commit interprets a None environment value as an empty string. It also ensures that all values are strings instead of, for example, `int`. It adds a paragraph to the doc-string of `datalad_next.config.utils.set_gitconfig_items_in_env` to document its behavior if a configuration item has a `None`-value. This commit is unrelated to Windows-support, but became necessary because the datalad-next CI runs datalad tests now always with the loaded `datalad_next`-extension, and the previous version of datalad_next's config code failed the tests.

christian-monch · 2024-05-13T14:51:57Z

Few first, very high-level comments:

The PR adds a number of patches that seem to replace all(?) of the ORARemote implementation -- or it is at least not easy to see which parts are left untouched. No[t] all individual patches are documented properly (top-level docstring explaining the purpose).
[...]
Please also at some indication where the, then modified, code was taken from. Further changes might be made in datalad-core, and someone would need to figure out which version to compare to. Grep for taken from in datalad_next/patches.
[...]

Thanks for the feedback. You are right, the source of the code and the amount of modifications are not obvious. I extended the patch to contain the following:

"taken from"-indicator that contains the commit from which code parts were taken
"# PATCH:"-comments that indicate where changes were made.
file doc-strings that explain the purpose of the patch.

[...]
This makes it hard to understand why these are individual patches, and not just one -- especially given the strong patch order requirements.

From what I can see, all of the following could be one patch (may still be composed of several files):

add_method_url2transport_path

replace_sshremoteio

ria_utils

replace_ora_remote

fix_ria_ora_tests

replace_create_sibling_ria
[...]

I see your point. I will create a single py-file that will be referenced in enabled.py and apply all patches. EDIT done.

This commit adds a new file that imports all patches for RIA/ORA-related code in a correct order. This file, i.e. `datalad_next.patches.patch_ria_ora.py`, is then imported from `datalad_next.patches.enabled`.

jsheunis · 2024-05-13T21:17:21Z

I have tested this on my mac locally, with the following setup (miniconda virtual environment):

datalad version 1.0.2
datalad-next version installed from this PR branch: 1.3.0+96.g3737a51
git-annex version: 10.20230227
Python: 3.12.3
System: darwin/20.6.0 10.16/x86_64

I ran the associated tests locally with no errors. I created a local datalad dataset with annexed content, and created a ria sibling in a ria store on the local file system as well as another sibling in a store accessible via SSH (juseless). Both commands ran without errors. datalad push to the siblings worked correctly. And then cloning the siblings to my local filesystem and getting content succeeded in both cases.

Is there anything else I should try out?

mih · 2024-05-14T06:21:44Z

I extracted the config None handling to #682 -- it has been failing the tests for too long, and there is no reason to block this fix.

christian-monch · 2024-05-14T06:49:47Z

I have tested this on my mac locally, with the following setup (miniconda virtual environment):

datalad version 1.0.2

datalad-next version installed from this PR branch: 1.3.0+96.g3737a51

git-annex version: 10.20230227

Python: 3.12.3

System: darwin/20.6.0 10.16/x86_64

I ran the associated tests locally with no errors. I created a local datalad dataset with annexed content, and created a ria sibling in a ria store on the local file system as well as another sibling in a store accessible via SSH (juseless). Both commands ran without errors. datalad push to the siblings worked correctly. And then cloning the siblings to my local filesystem and getting content succeeded in both cases.

Is there anything else I should try out?

Perfect, thanks a lot @jsheunis

If you have a Windows system and time on your hands, I would appreciate it if you could do a similar test there. I don't expect everything to work as well as in OSX because the focus was on cloning from a RIA store and getting content from the RIA store on a Windows system.

EDIT: tested on Windows: creation of local and SSH-RIA siblings, pushing to the siblings, cloning from the siblings, getting data from the siblings. Worked well.

Merge main into the PR-branch in order to activate appveyor CI-runs.

This commit ensures that Windows and non-Windows URL-canonification code is run on all platforms. This is possible because the code does not depend on platform-specific libraries.

mih

Thanks a lot for tackling this issue. The patch is a monster, but there was no other way. I am glad that its development also led to a generally more correct test behavior.

I tuned the docs a bit and added a changelog.

Thanks!

The patched code is new patch-code, introduced in datalad#669 The assertions were added to make sure that no platform paths are passed around. The Debian maintainer observed non-pure paths coming in. This may need an investigation, but this changeset maintains the spirit of the assertions while relaxing them enough to let the tests pass in a Debian build environment.

Since 1.4.0 a plain `datalad_next` import required `pytest`. This was due to the datalad-core patch `fix_ria_ora_tests` add in datalad#669, and was discovered by Debian's CI Closes: datalad#698 Refs: datalad#669, https://ci.debian.net/packages/d/datalad-next/testing/amd64/46884776/

@aqw

Since 1.4.0 a plain `datalad_next` import required `pytest`. This was due to the datalad-core patch `fix_ria_ora_tests` add in datalad#669, and was discovered by Debian's CI Thanks to @aqw for the recommendation. Closes: datalad#698 Refs: datalad#669, https://ci.debian.net/packages/d/datalad-next/testing/amd64/46884776/

@aqw

Since 1.4.0 a plain `datalad_next` import required `pytest`. This was due to the datalad-core patch `fix_ria_ora_tests` added in datalad#669, and was discovered by Debian's CI Thanks to @aqw for the recommendation. Closes: datalad#698 Refs: datalad#669, https://ci.debian.net/packages/d/datalad-next/testing/amd64/46884776/

@aqw

Since 1.4.0 a plain `datalad_next` import required `pytest`. This was due to the datalad-core patch `fix_ria_ora_tests` added in datalad#669, and was discovered by Debian's CI Thanks to @aqw for the recommendation. Closes: datalad#698 Refs: datalad#669, https://ci.debian.net/packages/d/datalad-next/testing/amd64/46884776/

christian-monch force-pushed the patch-create-sibling-ria branch 6 times, most recently from f2253bf to 80f7dee Compare May 3, 2024 12:53

christian-monch force-pushed the patch-create-sibling-ria branch from 29cc348 to 652f72c Compare May 6, 2024 11:40

christian-monch mentioned this pull request May 6, 2024

Handle global option -c properly #677

Open

mih linked an issue May 7, 2024 that may be closed by this pull request

ORA remote initialization stalls on windows #654

Closed

christian-monch marked this pull request as ready for review May 10, 2024 07:38

christian-monch requested a review from mih as a code owner May 10, 2024 07:38

mih self-assigned this May 13, 2024

mih reviewed May 13, 2024

View reviewed changes

mih assigned christian-monch and unassigned mih May 13, 2024

christian-monch force-pushed the patch-create-sibling-ria branch from ac7eecc to 36de721 Compare May 13, 2024 08:58

mih reviewed May 13, 2024

View reviewed changes

datalad_next/patches/fix_ria_ora_tests.py Show resolved Hide resolved

christian-monch added 3 commits May 13, 2024 15:45

christian-monch force-pushed the patch-create-sibling-ria branch from 36de721 to c929360 Compare May 13, 2024 13:47

christian-monch added 4 commits May 13, 2024 16:35

adapt tests to PurePosixPath-arguments in RIA/OR

140478c

This commit adapts tests that use RIA- and ORA-code to use the internal RIA-path representation, i.e. `PurePosixPath`. That has become necessary because the patched RIA code asserts that path-arguments are instances of `PurePosixPath`.

active ria_roundtrip-test on Windows

4df90fb

This commit activates the test `datalad_next.patches.tests.test_ria.test_ria_ssh_roundtrip` on Windows. With the previous commits in this branch, the test should now pass.

christian-monch force-pushed the patch-create-sibling-ria branch from c929360 to 325d89a Compare May 13, 2024 14:52

create a single patch-file for all RIA/ORA patches

3737a51

This commit adds a new file that imports all patches for RIA/ORA-related code in a correct order. This file, i.e. `datalad_next.patches.patch_ria_ora.py`, is then imported from `datalad_next.patches.enabled`.

mih mentioned this pull request May 14, 2024

Fix no-HEAD handling in iter_gitstatus() #681

Merged

exclude froms-parameter from codespell's checks

558640a

christian-monch force-pushed the patch-create-sibling-ria branch from a7ffccd to 558640a Compare May 14, 2024 07:17

christian-monch added 5 commits May 14, 2024 09:53

Merge branch 'main' into patch-create-sibling-ria

85e9be1

Merge main into the PR-branch in order to activate appveyor CI-runs.

run all URL-canonification tests on all platforms

dcc6557

This commit ensures that Windows and non-Windows URL-canonification code is run on all platforms. This is possible because the code does not depend on platform-specific libraries.

add tests for url2transport_path-methods

a76089e

Merge branch 'main' into patch-create-sibling-ria

3074c2f

fix Windows tests for LocalIO.url2transport_path

585b48d

christian-monch force-pushed the patch-create-sibling-ria branch from 9955ae6 to 585b48d Compare May 14, 2024 11:19

christian-monch requested a review from mih May 14, 2024 12:24

mih added 2 commits May 14, 2024 21:23

List RIA patches in docs

75f8449

Add changelog snippet for RIA update

2a6772a

mih approved these changes May 14, 2024

View reviewed changes

mih merged commit 096a567 into datalad:main May 14, 2024
4 of 5 checks passed

mih added this to the 1.4 milestone May 15, 2024

christian-monch mentioned this pull request May 15, 2024

Keep repository paths system independent (or POSIX compatible) in create-sibling-ria and ora-remote code datalad/datalad-ria#108

Closed

This was referenced May 20, 2024

Relax type assertion from PurePosixPath to PosixPath in RIA patches #696

Merged

pytest now unintentional runtime dependency #698

Closed

mih mentioned this pull request May 21, 2024

fix(dependencies): limit test patch import to test runs #701

Merged

mih mentioned this pull request May 21, 2024

[WARNING] Could not load extension 'next': ImportError(cannot import name 'Dataset' from partially initialized module 'datalad.api' #703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patch RIA and ORA code to work on multiple client platforms #669

Patch RIA and ORA code to work on multiple client platforms #669

christian-monch commented Apr 25, 2024 •

edited

codecov bot commented May 7, 2024 •

edited

mih left a comment

christian-monch commented May 13, 2024 •

edited

jsheunis commented May 13, 2024

mih commented May 14, 2024

christian-monch commented May 14, 2024 •

edited

mih left a comment

Patch RIA and ORA code to work on multiple client platforms #669

Patch RIA and ORA code to work on multiple client platforms #669

Conversation

christian-monch commented Apr 25, 2024 • edited

What does it do?

TODO

codecov bot commented May 7, 2024 • edited

Codecov Report

mih left a comment

Choose a reason for hiding this comment

christian-monch commented May 13, 2024 • edited

jsheunis commented May 13, 2024

mih commented May 14, 2024

christian-monch commented May 14, 2024 • edited

mih left a comment

Choose a reason for hiding this comment

christian-monch commented Apr 25, 2024 •

edited

codecov bot commented May 7, 2024 •

edited

christian-monch commented May 13, 2024 •

edited

christian-monch commented May 14, 2024 •

edited