jupyter-server / jupyter_server_fileid

An extension that maintains file IDs for documents in a running Jupyter Server
https://jupyter-server-fileid.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
3 stars 12 forks source link

support migrations between different file ID managers #49

Open dlqqq opened 1 year ago

dlqqq commented 1 year ago

Description

The key principle is that the previous FIDM defines how to export the rows in its database, and the current FIDM defines how to import the rows into its database.

Limitations

Open questions

Related issues

dlqqq commented 1 year ago

I will need consensus on whether we're OK with imposing the constraint that all File ID manager implementations use SQLite. It's not clear if a general migration strategy would be possible otherwise. Furthermore, if we impose this constraint, we can merge the duplicate __init__() logic in ArbitraryFIDM and LocalFIDM into BaseFIDM, and then perform migrations for all custom FIDMs automatically (as long as custom FIDMs meaningfully implement the import and export methods).

codecov-commenter commented 1 year ago

Codecov Report

Base: 85.74% // Head: 85.48% // Decreases project coverage by -0.25% :warning:

Coverage data is based on head (6fca139) compared to base (50dec2e). Patch coverage: 86.99% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #49 +/- ## ========================================== - Coverage 85.74% 85.48% -0.26% ========================================== Files 4 4 Lines 519 620 +101 Branches 68 86 +18 ========================================== + Hits 445 530 +85 - Misses 52 61 +9 - Partials 22 29 +7 ``` | [Impacted Files](https://codecov.io/gh/jupyter-server/jupyter_server_fileid/pull/49?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server) | Coverage Δ | | |---|---|---| | [jupyter\_server\_fileid/manager.py](https://codecov.io/gh/jupyter-server/jupyter_server_fileid/pull/49/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server#diff-anVweXRlcl9zZXJ2ZXJfZmlsZWlkL21hbmFnZXIucHk=) | `89.16% <86.99%> (-1.31%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jupyter-server)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

dlqqq commented 1 year ago

Interesting edge case when migrating from a local FIDM to arbitrary FIDM on Windows. Essentially, local FIDM knows that this is a local Windows path, and calls os.path.normpath() on it to map it to lowercase (since there's no distinction b/w upper and lower case in Windows filesystems).

However, arbitrary FIDM doesn't make any assumptions of the local filesystem and is case sensitive even when running on Windows, and hence is not able to return a relative path since it sees the lower-case content root as being different from the content root.

_______________________ test_migrate_local_to_arbitrary _______________________

fid_db_path = 'C:\\Users\\runneradmin\\AppData\\Local\\Temp\\pytest-of-runneradmin\\pytest-0\\test_migrate_local_to_arbitrar0\\data\\fileidmanager_test.db'
jp_root_dir = WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/test_migrate_local_to_arbitrar0/root_dir')
test_path = 'test_path', test_path_child = 'test_path/child'

    def test_migrate_local_to_arbitrary(fid_db_path, jp_root_dir, test_path, test_path_child):
        local = LocalFileIdManager(db_path=fid_db_path, root_dir=str(jp_root_dir))
        local.con.execute("PRAGMA journal_mode = off")

        id_1 = local.index(test_path)
        id_2 = local.index(test_path_child)
        del local
        arbitrary = ArbitraryFileIdManager(db_path=fid_db_path, root_dir=str(jp_root_dir))

>       assert arbitrary.get_path(id_1) == test_path
E       AssertionError: assert 'c:/users/runneradmin/appdata/local/temp/pytest-of-runneradmin/pytest-0/test_migrate_local_to_arbitrar0/root_dir/test_path' == 'test_path'
E         - test_path
E         + c:/users/runneradmin/appdata/local/temp/pytest-of-runneradmin/pytest-0/test_migrate_local_to_arbitrar0/root_dir/test_path
dlqqq commented 1 year ago

I just patched the test to fix the above error. I'm not sure if this edge case is worth handling, given that the motivation is unclear (downgrading on a local filesystem) and that the workaround is as simple as providing a normalized contents root on Windows.