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.

Sign up for GitHub

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

Jump to bottom

Load the cli module lazily for spacy.info #12962

Merged
merged 2 commits into from
Sep 28, 2023
Merged

Load the cli module lazily for spacy.info #12962

merged 2 commits into from
Sep 28, 2023

Conversation

danieldk
Copy link
Contributor

@danieldk danieldk commented Sep 6, 2023

Description

This avoids that the spacy module cannot be imported when the users chooses not to install typer/requests.

Proposed alternative to #12952. With lazy loading, people that do have typer/requests installed will not pay the loading penalty when they just import import spacy.

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.

@danieldk danieldk added enhancement Feature requests and improvements compat Cross-platform and cross-Python compatibility labels Sep 6, 2023
@danieldk
Load the cli module lazily for spacy.info
bff70d8 
This avoids that the `spacy` module cannot be imported when the
users chooses not to install `typer`/`requests`.
@danieldk danieldk force-pushed the maintenance/lazy-load-cli branch from 70dd8b9 to bff70d8 Compare September 7, 2023 07:29
@Vuizur
Copy link

Vuizur commented Sep 7, 2023

I think lazy importing in general is a good solution, I used https://github.com/orsinium-labs/benchmark-imports and importing the CLI and requests apparently takes quite a while:

1.1127 root       spacy
0.4362 project    spacy.errors
0.4353 project    spacy.compat
0.4066 project    spacy.cli
0.3396 dependency thinc
0.2671 project    spacy.pipeline
0.2066 project    spacy.cli.download
0.2055 dependency requests
0.2004 transitive thinc.config                             from thinc
0.1934 project    spacy.pipeline.attributeruler
0.1479 transitive confection                               from thinc.config
0.1360 transitive numpy                                    from thinc
0.1310 project    spacy.language
0.0842 transitive urllib3                                  from requests
0.0736 transitive requests.exceptions                      from requests
0.0734 dependency thinc.api
0.0729 transitive requests.compat                          from requests.exceptions
0.0656 transitive pydantic                                 from confection
0.0646 project    spacy.pipe_analysis
0.0644 transitive charset_normalizer                       from requests.compat
0.0637 project    spacy.tokens
0.0629 project    spacy.tokens._serialize
0.0578 project    spacy.util
0.0499 project    spacy.cli.debug_diff
0.0495 transitive charset_normalizer.api                   from charset_normalizer

@adrianeboyd
Copy link
Contributor

I had actually never noticed spacy.info in the API and it looks like this isn't tested anywhere. I think it would make sense to add a very quick test that it runs without errors, maybe in test_cli_info? This isn't 100% optimal (I might have chosen test_misc otherwise), but it would be nice to test it with args without creating another tmpdir?

@adrianeboyd adrianeboyd merged commit beda27a into explosion:master Sep 28, 2023
@omri374 omri374 mentioned this pull request Oct 4, 2023
Closed
adrianeboyd added a commit to adrianeboyd/spaCy that referenced this pull request Oct 4, 2023
@adrianeboyd
Revert "Load the cli module lazily for spacy.info (explosion#12962)"
6d0185f 
This reverts commit beda27a.
adrianeboyd added a commit that referenced this pull request Oct 4, 2023
@adrianeboyd
Merge pull request #13040 from adrianeboyd/revert/12962-spacy-info
aec59c0 
Revert "Load the cli module lazily for spacy.info (#12962)"
@nro-bot nro-bot mentioned this pull request Dec 19, 2024
Draft
2 tasks
@hoodmane hoodmane mentioned this pull request Dec 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
compat Cross-platform and cross-Python compatibility enhancement Feature requests and improvements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danieldk @Vuizur @adrianeboyd