Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#27916 closed enhancement (fixed)

using more lazy imports

Reported by: Frédéric Chapoton Owned by:
Priority: major Milestone: sage-8.9
Component: refactoring Keywords:
Cc: Erik Bray, Jeroen Demeyer, François Bissey, Travis Scrimshaw, John Palmieri, vklein Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 2cfa486 (Commits, GitHub, GitLab) Commit: 2cfa48617c7af290330d950a846f7325a1283181
Dependencies: Stopgaps:

Status badges

Description

in order to avoid importing urllib at startup

Change History (22)

comment:1 Changed 3 years ago by Frédéric Chapoton

Branch: u/chapoton/27916
Commit: 00094d011d48115e66c5a4ba83fbfca5a2eba65e
Status: newneeds_review

New commits:

00094d0more lazy imports, to avoid import of urllib at startup

comment:2 Changed 3 years ago by Frédéric Chapoton

Status: needs_reviewneeds_work

comment:3 Changed 3 years ago by git

Commit: 00094d011d48115e66c5a4ba83fbfca5a2eba65e08971b10174e0b6081928db3fc88c5017b729776

Branch pushed to git repo; I updated commit sha1. New commits:

08971b1fix some mistakes

comment:4 Changed 3 years ago by git

Commit: 08971b10174e0b6081928db3fc88c5017b7297765e81335087a4146e200bc5224dad14ff0f41f1d5

Branch pushed to git repo; I updated commit sha1. New commits:

5e81335fix

comment:5 Changed 3 years ago by git

Commit: 5e81335087a4146e200bc5224dad14ff0f41f1d5ef06b0d60f1d184b8eb38ed5c212d0e3f96baf9d

Branch pushed to git repo; I updated commit sha1. New commits:

ef06b0dtrac 27916 one more fix

comment:6 Changed 3 years ago by git

Commit: ef06b0d60f1d184b8eb38ed5c212d0e3f96baf9d2cfa48617c7af290330d950a846f7325a1283181

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2cfa486more lazy imports, to avoid import of urllib at startup

comment:7 Changed 3 years ago by Frédéric Chapoton

Cc: Erik Bray Jeroen Demeyer François Bissey Travis Scrimshaw added

green bot, please review

comment:8 Changed 3 years ago by Frédéric Chapoton

Status: needs_workneeds_review

comment:9 Changed 3 years ago by Frédéric Chapoton

Cc: John Palmieri vklein added

Please review, somebody ? This cuts off slightly sage startup time.

comment:10 Changed 3 years ago by Travis Scrimshaw

I am a little worried about multiple lazy_import resolutions of the designs stuff. I am wondering if it might be better to lazily import the whole catalog instead. Or do you think I am being paranoid or that we should take the more granular approach?

comment:11 Changed 3 years ago by Frédéric Chapoton

I think that I tried that first, and was forced to the current solution. I suspect that the line

from . import design_catalog as designs

may be preventing the lazy import. Let me try again.

comment:12 Changed 3 years ago by git

Commit: 2cfa48617c7af290330d950a846f7325a12831813c809588f7c5e7414629fdf1850c876019090eed

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

3c80958more lazy imports, to avoid import of urllib at startup

comment:13 Changed 3 years ago by Frédéric Chapoton

This seems to work, let us see if the patchbot turns green.

comment:14 Changed 3 years ago by Travis Scrimshaw

That seems to cause massive breakage on one of the patchbots. :/

comment:15 Changed 3 years ago by Frédéric Chapoton

Branch: u/chapoton/27916u/chapoton/27916_original
Commit: 3c809588f7c5e7414629fdf1850c876019090eed2cfa48617c7af290330d950a846f7325a1283181

ok, this was the reason for not doing it. Let me put back my previous branch..


New commits:

2cfa486more lazy imports, to avoid import of urllib at startup

comment:16 Changed 3 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

Then so be it.

comment:17 Changed 3 years ago by Erik Bray

You write that this is all to avoid importing urllib. But is urllib a major contributor to start-up time? lazy_import has its own overhead. Less directly than actually importing one module in many cases. But is adding more lazy_imports actually better in this case than avoiding just one import (urllib)?

Note, I'm all for more lazy_import in general so I'm not changing the ticket status, but I don't understand the motivation here or if it's justified.

comment:18 Changed 3 years ago by Frédéric Chapoton

I used sage -startuptime to see what imported modules could be avoided without too much work. I have seen in the list email and http. Tracking who imported them lead to urllib.

comment:19 Changed 3 years ago by Erik Bray

Seems like a bit overkill. Just loading plain IPython causes both the email and urllib modules to be loaded (but not urllib.requests which is showing up in ./sage -startuptime). Running the Jupyter notebook is almost certain to ultimately use the others.

Does this definitely shave anything noticeable off the startup time (again, all for it if it does)?

comment:20 Changed 3 years ago by Erik Bray

Answering my own question, I noticed an improvement with this patch of ~100-150ms over without it, pretty consistently. In this game every bit counts, so, +1.

comment:21 Changed 3 years ago by Volker Braun

Branch: u/chapoton/27916_original2cfa48617c7af290330d950a846f7325a1283181
Resolution: fixed
Status: positive_reviewclosed

comment:22 Changed 3 years ago by Erik Bray

Milestone: sage-8.8sage-8.9

Not in Sage 8.8. Let's please to try keep tickets' milestones related to the release in which we actually intend to include them, and in particular the release in which they were actually included, especially when closing tickets.

Note: See TracTickets for help on using tickets.