Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch from pathy to cloudpathlib #11750

Closed

Conversation

adrianeboyd
Copy link
Contributor

@adrianeboyd adrianeboyd commented Nov 4, 2022

Description

Switch from pathy to cloudpathlib for remote storage with spacy project push/pull.

  • Continue using smart_open for downloads, which are also used for project assets.
  • Switch to cloudpathlib for uploads.
  • Replace deprecated ignore_ext flag with compression and add corresponding smart_open version requirements.
  • Add tests for local filesystem remote storage.
  • Add functionality for sorting multiple content hashes by last modified.

Types of change

Enhancement.

Checklist

  • I confirm that I have the right to submit this contribution under the project's MIT license.
  • I ran the tests, and all new and existing tests passed.
  • My changes don't require a change to the documentation, or if they do, I've added all required information.

@adrianeboyd adrianeboyd added the ⚠️ wip Work in progress label Nov 4, 2022
@adrianeboyd adrianeboyd marked this pull request as draft November 4, 2022 14:50
spacy/cli/_util.py Outdated Show resolved Hide resolved
@adrianeboyd adrianeboyd changed the title Demo: Switch from pathy to cloudpathlib Switch from pathy to cloudpathlib Nov 7, 2022
@adrianeboyd adrianeboyd added feat / cli Feature: Command-line interface and removed ⚠️ wip Work in progress labels Nov 7, 2022
@adrianeboyd adrianeboyd marked this pull request as ready for review November 7, 2022 13:43
@adrianeboyd adrianeboyd added the v3.5 Related to v3.5 label Nov 7, 2022
Copy link
Contributor

@polm polm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question about the changes but looks good.

spacy/cli/_util.py Show resolved Hide resolved
@polm polm mentioned this pull request Nov 10, 2022
6 tasks
@adrianeboyd
Copy link
Contributor Author

The test should probably be marked slow in the end.

Copy link
Contributor

@polm polm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like #11780 introduced a minor conflict in the docs, so I went ahead and resolved that.

Besides that the most recent change looks fine.

polm added a commit to koaning/weasel that referenced this pull request Nov 15, 2022
@adrianeboyd adrianeboyd marked this pull request as draft November 25, 2022 10:35
@adrianeboyd
Copy link
Contributor Author

I'll close this for now since the issues with pathy were resolved. We can always come back to it if we need to...

@BLKSerene
Copy link
Contributor

Hi, I noticed that spaCy 3.7 introduced weasel as a dependency which depends on cloudpathlib instead of pathy. Would it be more consistent and easier for the dependency management of spaCy to switch to cloudpathlib as well?

@adrianeboyd
Copy link
Contributor Author

Yes it would, thanks for the note! Neither of them currently support python 3.12, however.

@adrianeboyd
Copy link
Contributor Author

Actually, Pathy should have just been removed from spacy, it's no longer used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat / cli Feature: Command-line interface v3.5 Related to v3.5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants