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

Redesigned cython profiling and other minor updates for python 3.12 #12979

Merged
merged 7 commits into from
Sep 29, 2023

Conversation

adrianeboyd
Copy link
Contributor

Description

Swap profile default to True and disable entirely in setup for python 3.12+.

  • Add profile=False to currently unprofiled cython
  • Remove profile=True from currently profiled cython
  • Set cython profiling default to True for <3.12, False for >=3.12

Types of change

?

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 compat Cross-platform and cross-Python compatibility v3.7 Related to v3.7 labels Sep 13, 2023
@adrianeboyd
Copy link
Contributor Author

This is intended to preserve the current behavior. We could potentially consider disabling profiling entirely by default.

@rmitsch
Copy link
Contributor

rmitsch commented Sep 15, 2023

This is intended to preserve the current behavior. We could potentially consider disabling profiling entirely by default.

Why would we disable it - wouldn't it better to stick with the current behavior?

@adrianeboyd
Copy link
Contributor Author

Most people never use profiling and there are slight performance hits, so it could be reasonable to have the default published wheels have no profiling, and have across-the-board profiling be an option that you can enable when compiling from source. (Managing that kind of option for pip through setup.py is probably kind of a hassle, though.)

@rmitsch
Copy link
Contributor

rmitsch commented Sep 18, 2023

(Managing that kind of option for pip through setup.py is probably kind of a hassle, though.)

In that case I'd stick with the current behavior.

@adrianeboyd adrianeboyd changed the title Redesign cython profiling due to python 3.12 Redesign cython profiling and other minor updates for python 3.12 Sep 22, 2023
@adrianeboyd adrianeboyd changed the title Redesign cython profiling and other minor updates for python 3.12 Redesigned cython profiling and other minor updates for python 3.12 Sep 22, 2023
@adrianeboyd
Copy link
Contributor Author

adrianeboyd commented Sep 25, 2023

A short summary of the problems with tqdm before I force-push to clean up the history a bit:

  • tqdm v4.66.1 produces a deprecation warning starting in python 3.12, which is then raised to an error in the test suite
  • when the warning is raised to an error in the test suite, the test suite deadlocks consistently when both these tests are run:
    • spacy/tests/test_cli.py::test_applycli_docbin
    • spacy/tests/test_cli.py::test_cli_find_threshold

As in some older uses of tqdm in spacy, it looks like the best workaround is use disable=None so that tqdm output is disabled in the test suite and other non-interactive uses. This is not a solution for the underlying problem with deadlocks in tqdm (looking into the details makes me want to take tqdm out of spacy entirely, I feel like it's not really needed and would at least be better if it's entirely optional), but avoids the deadlocks and other problems related to raising errors in many everyday situations.

The test suite will not pass until the next version of weasel is released with some 3.12 updates and the remote tests are disabled in spacy.

@adrianeboyd adrianeboyd force-pushed the feature/cython-profile-312 branch 3 times, most recently from 5616966 to 6b8f91f Compare September 25, 2023 06:16
@adrianeboyd adrianeboyd merged commit 4ec41e9 into explosion:develop Sep 29, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compat Cross-platform and cross-Python compatibility v3.7 Related to v3.7
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants