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

Delay import of scipy stats and optimize #4643

Merged
merged 8 commits into from
Nov 9, 2021
Merged

Conversation

dabacon
Copy link
Collaborator

@dabacon dabacon commented Nov 9, 2021

This speeds up top level import.

Note that we had previously done this for another use of scipy.stats, but this one got added. Also doing it for scipy.optimize. Both of these can slow down the resulting experiments, probably a case for moving experiments out of cirq-core?

@dabacon dabacon requested review from cduck, mrwojtek, vtomole and a team as code owners November 9, 2021 07:05
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Nov 9, 2021
@CirqBot CirqBot added the size: S 10< lines changed <50 label Nov 9, 2021
@dabacon
Copy link
Collaborator Author

dabacon commented Nov 9, 2021

python -c 'import time; time.sleep(1); t1 = time.monotonic(); import cirq; t2 = time.monotonic(); print(t2-t1)'

Before the PR gives 1.7412576620699838 and after gives 1.355757946963422

@dabacon
Copy link
Collaborator Author

dabacon commented Nov 9, 2021

Current slowness besides these scipy modules are
sympy: 0.386s
numpy: 0.169s
pands: 0.204s
matplotlib.pyplot 0.334s
networkx: 0.079s

@MichaelBroughton MichaelBroughton self-assigned this Nov 9, 2021
@MichaelBroughton
Copy link
Collaborator

MichaelBroughton commented Nov 9, 2021

This looks good. Did you have the time to checkout something like this: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/util/lazy_loader.py#L23 at all ?

Which would give us the added bonus of being able to start rounding up all the random imports we have littered everywhere to avoid circular imports (putting them back at the top with this) ?

@dabacon
Copy link
Collaborator Author

dabacon commented Nov 9, 2021

rounding

Untested TensorFlow code! My eyes they burn! The googles they do nothing! Do you see the benefit of the TensorFlow class? I guess if you use the class in a lot of places?

@dabacon dabacon requested a review from wcourtney as a code owner November 9, 2021 15:09
@MichaelBroughton
Copy link
Collaborator

Do you see the benefit of the TensorFlow class?

Well you don't have to worry about where in the module you'd need to put your imports if you used this class. The actual load would take place when the call is made. So in this case of scipy instead of having to put multiple imports in weid places just do at the top:

# can do circular imports at the top with this too.
scipy_linalg = LazyLoader("scipy.whatever", globals())

def my_function(...):
   stuff.

def my_scipy_function(...):
  return scipy_linalg.whatever # this triggers actual import.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@dabacon
Copy link
Collaborator Author

dabacon commented Nov 9, 2021

Created #4648 to consider adding LazyLoader.

Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

LGTM, we should probably hold off on more like this for now until a decision is made on the LazyLoader approach (otherwise we might just be making more work for ourselves here).

@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Nov 9, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 9, 2021
@CirqBot CirqBot merged commit 321ce63 into quantumlib:master Nov 9, 2021
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Nov 9, 2021
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Nov 9, 2021
@dabacon dabacon deleted the stats branch April 16, 2022 15:42
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
This speeds up top level import.

Note that we had previously done this for another use of scipy.stats, but this one got added.  Also doing it for scipy.optimize.  Both of these can slow down the resulting experiments, probably a case for moving experiments out of cirq-core?
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
This speeds up top level import.

Note that we had previously done this for another use of scipy.stats, but this one got added.  Also doing it for scipy.optimize.  Both of these can slow down the resulting experiments, probably a case for moving experiments out of cirq-core?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining. size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants