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

Delay import of sympy until actually needed. #194

Merged
merged 1 commit into from Jan 28, 2020
Merged

Delay import of sympy until actually needed. #194

merged 1 commit into from Jan 28, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 27, 2020 •
edited

sympy is a rather slow import (~330ms for me) which contributes nearly
half of bezier's import time (~750ms). Delay the import until actually
needed.

As a typical use case where this is visible: I have a command-line
utility which does some computation using bezier. Right now invoking
something as simple as my-program --help takes over a second to
complete (certainly a noticeable delay), in particular due to such slow
imports -- even if I do not use the symbolic computation part at all.

Obviously there are other ways in which this can be implemented, e.g. by
duplicating the try... except ImportError in each function, or with
another helper function, but given that you already have a
requires_sympy decorator, I thought I may as well reuse it for this
purpose.

Edit: The decorator magic apparently makes pylint/flake8 extremely unhappy. I can fix that with whichever approach you prefer -- skip linting on the offending lines, or just duplicate the try: except import everywhere, or a helper function.

@anntzer anntzer force-pushed the delaysympy branch 3 times, most recently from db50cae to 6327459 Compare January 27, 2020 11:23
@anntzer anntzer mentioned this pull request Jan 27, 2020
Merged
dhermes
dhermes reviewed Jan 27, 2020
View reviewed changes
src/python/bezier/_symbolic.py Outdated

return ensure_sympy


@require_sympy
def to_symbolic(nodes):
def to_symbolic(sympy, nodes):
Copy link
Owner

@dhermes dhermes Jan 27, 2020 •
edited

Choose a reason for hiding this comment

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

This would mean Args needs to be updated for this function. (Once upon a time Pylint would've flagged the mismatch, not sure what the difference is has changed.)

Copy link
Owner

Choose a reason for hiding this comment

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

Ah I spoke too soon, lint failure from this PR:

************* Module bezier._symbolic
src/python/bezier/_symbolic.py:52:0: W9015: "sympy" missing in parameter documentation (missing-param-doc)
src/python/bezier/_symbolic.py:52:0: W9016: "sympy" missing in parameter type documentation (missing-type-doc)
src/python/bezier/_symbolic.py:73:0: W9015: "sympy" missing in parameter documentation (missing-param-doc)
src/python/bezier/_symbolic.py:73:0: W9016: "sympy" missing in parameter type documentation (missing-type-doc)
src/python/bezier/_symbolic.py:98:0: W9015: "sympy" missing in parameter documentation (missing-param-doc)
src/python/bezier/_symbolic.py:98:0: W9016: "sympy" missing in parameter type documentation (missing-type-doc)
src/python/bezier/_symbolic.py:111:16: E1120: No value for argument 'nodes' in function call (no-value-for-parameter)
src/python/bezier/_symbolic.py:114:31: E1120: No value for argument 's' in function call (no-value-for-parameter)
src/python/bezier/_symbolic.py:122:0: W9015: "sympy" missing in parameter documentation (missing-param-doc)
src/python/bezier/_symbolic.py:122:0: W9016: "sympy" missing in parameter type documentation (missing-type-doc)
src/python/bezier/_symbolic.py:139:0: W9015: "sympy" missing in parameter documentation (missing-param-doc)
src/python/bezier/_symbolic.py:139:0: W9016: "sympy" missing in parameter type documentation (missing-type-doc)
src/python/bezier/_symbolic.py:157:22: E1120: No value for argument 'degree' in function call (no-value-for-parameter)
src/python/bezier/_symbolic.py:159:11: E1120: No value for argument 's' in function call (no-value-for-parameter)
src/python/bezier/_symbolic.py:139:22: W0613: Unused argument 'sympy' (unused-argument)
src/python/bezier/_symbolic.py:163:0: W9015: "sympy" missing in parameter documentation (missing-param-doc)
src/python/bezier/_symbolic.py:163:0: W9016: "sympy" missing in parameter type documentation (missing-type-doc)
src/python/bezier/_symbolic.py:197:0: W9015: "sympy" missing in parameter documentation (missing-param-doc)
src/python/bezier/_symbolic.py:197:0: W9016: "sympy" missing in parameter type documentation (missing-type-doc)
src/python/bezier/_symbolic.py:211:16: E1120: No value for argument 'nodes' in function call (no-value-for-parameter)
src/python/bezier/_symbolic.py:214:31: E1120: No value for argument 't' in function call (no-value-for-parameter)
src/python/bezier/_symbolic.py:222:0: W9015: "sympy" missing in parameter documentation (missing-param-doc)
src/python/bezier/_symbolic.py:222:0: W9016: "sympy" missing in parameter type documentation (missing-type-doc)
src/python/bezier/_symbolic.py:246:0: W9015: "sympy" missing in parameter documentation (missing-param-doc)
src/python/bezier/_symbolic.py:246:0: W9016: "sympy" missing in parameter type documentation (missing-type-doc)
src/python/bezier/_symbolic.py:264:25: E1120: No value for argument 'degree' in function call (no-value-for-parameter)
src/python/bezier/_symbolic.py:266:11: E1120: No value for argument 't' in function call (no-value-for-parameter)
src/python/bezier/_symbolic.py:246:25: W0613: Unused argument 'sympy' (unused-argument)
************* Module bezier.triangle
src/python/bezier/triangle.py:1180:29: E1120: No value for argument 'degree' in function call (no-value-for-parameter)
src/python/bezier/triangle.py:1218:15: E1120: No value for argument 'degree' in function call (no-value-for-parameter)
************* Module bezier.curve
src/python/bezier/curve.py:764:26: E1120: No value for argument 'degree' in function call (no-value-for-parameter)
src/python/bezier/curve.py:800:15: E1120: No value for argument 'degree' in function call (no-value-for-parameter)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it's not really an argument -- callers don't need to pass it, the decorator handles it.

I guess getting rid of requires_sympy and having something like

def _load_sympy():
    try: import sympy
    except ImportError: raise OSError(...)
    else: return sympy

def to_symbolic(nodes):
    sympy = _load_sympy()
    ...

would minimize repetition which remaining linter-friendly?

@dhermes
Copy link
Owner

dhermes commented Jan 27, 2020

Will try to think about what I'd "prefer" to make Pylint happy.

While we're on this "avoid imports" kick, does the SciPy import also slow things down? It's only needed for the pure Python implementations, so if you've got a wheel (or you manually built the Fortran speedups from source) you never need SciPy.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 27, 2020

Sure, looks like (in a completely clean venv) the presence of scipy slows down importing bezier from ~150ms to 240ms, so while it's not as much as sympy, it's definitely quite significant. I can prepare a PR once we figure out the best pattern to deal with sympy.

@dhermes
Copy link
Owner

dhermes commented Jan 27, 2020 •
edited

Somewhat related (i.e. caused builds in #195 to fail): I pushed a change to avoid pushing to coveralls for non-master branches (5b8e93b). Probably could've opened it up a bit and just checked CIRCLE_PR_NUMBER / CIRCLE_PULL_REQUEST / CIRCLE_PULL_REQUESTS.

Merge master in here (or rebase, which is my personal preferred strategy).

@dhermes dhermes self-assigned this Jan 27, 2020
@anntzer
Copy link
Contributor Author

anntzer commented Jan 27, 2020

I can rebase, but let's first figure out how you prefer me to import sympy?

@dhermes
Copy link
Owner

dhermes commented Jan 27, 2020 •
edited

So one of the things that's throwing off Pylint is the fact that the decorator "swallows" one of the arguments. I'm not a fan of magic like that either, so rather than a decorator, we could at least consider starting each function with

sympy = require_sympy()

(and changing require_sympy() to just return sympy)

In _plot_helpers.py I didn't even both with a helpful error message on ImportError, I just tried the import at function call time. (I wrote _plot_helpers.py a long time ago.)

@anntzer
Copy link
Contributor Author

anntzer commented Jan 27, 2020

basically #194 (comment)?

@dhermes
Copy link
Owner

dhermes commented Jan 27, 2020

basically #194 (comment)?

Haha absolutely yes!

@anntzer
Copy link
Contributor Author

anntzer commented Jan 27, 2020

done

@anntzer anntzer mentioned this pull request Jan 27, 2020
Closed
@anntzer
Delay import of sympy until actually needed.
18d2228 
sympy is a rather slow import (~330ms for me) which contributes nearly
half of bezier's import time (~750ms).  Delay the import until actually
needed.

As a typical use case where this is visible: I have a command-line
utility which does some computation using bezier.  Right now invoking
something as simple as `my-program --help` takes over a second to
complete (certainly a noticeable delay), in particular due to such slow
imports -- even if I do not use the symbolic computation part at all.

Obviously there are other ways in which this can be implemented, e.g. by
duplicating the `try... except ImportError` in each function, or with
another helper function, but given that you already have a
requires_sympy decorator, I thought I may as well reuse it for this
purpose.
dhermes
dhermes approved these changes Jan 27, 2020
View reviewed changes
Copy link
Owner

@dhermes dhermes left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm feeling like we can ditch require_sympy() altogether. Do you think it's a bad user experience to get an ImportError with no explanation why your dependency is missing?

@anntzer
Copy link
Contributor Author

anntzer commented Jan 27, 2020 •
edited

I actually think an ImportError is nicer than an OSError -- if I have a missing package, I'd rather have the "natural" exception being thrown (especially that with recent Pythons you get an even more accurate ModuleNotFoundError), rather than some wrapper around it.
In any case, I'm happy to implement whichever strategy you prefer.

@dhermes
Copy link
Owner

dhermes commented Jan 28, 2020

Good deal! So let's dump the require_sympy() wrapper altogether?

@dhermes dhermes merged commit fb4d416 into dhermes:master Jan 28, 2020
@dhermes
Copy link
Owner

dhermes commented Jan 28, 2020

@anntzer I just went ahead and merged as-is, thanks for pointing out this import-time issue. I'll follow up to get rid of the "extra" needed to make things work. I can do the same and obsolete #196 if you don't mind?

dhermes added a commit that referenced this pull request Jan 28, 2020
@dhermes
Moving scipy imports to happen at runtime.
38602d8 
Follows along approach from previous commit and earlier work
in #194, #195. This supercedes #196.

This also fixes a "SympPy" -> "SymPy" typo from the previous commit.
@anntzer
Copy link
Contributor Author

anntzer commented Jan 28, 2020 •
edited

Please do :)
Edit: thanks for taking care of everything.

@anntzer anntzer deleted the delaysympy branch January 28, 2020 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhermes dhermes dhermes approved these changes

Assignees

@dhermes dhermes

Labels
language: python packaging performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@anntzer @dhermes