Opened 23 months ago

Closed 22 months ago

Last modified 22 months ago

#27916 closed enhancement (fixed)

using more lazy imports

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.9
Component: refactoring Keywords:
Cc: embray, jdemeyer, fbissey, tscrim, jhpalmieri, 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 23 months ago by chapoton

  • Branch set to u/chapoton/27916
  • Commit set to 00094d011d48115e66c5a4ba83fbfca5a2eba65e
  • Status changed from new to needs_review

New commits:

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

comment:2 Changed 23 months ago by chapoton

  • Status changed from needs_review to needs_work

comment:3 Changed 23 months ago by git

  • Commit changed from 00094d011d48115e66c5a4ba83fbfca5a2eba65e to 08971b10174e0b6081928db3fc88c5017b729776

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

08971b1fix some mistakes

comment:4 Changed 23 months ago by git

  • Commit changed from 08971b10174e0b6081928db3fc88c5017b729776 to 5e81335087a4146e200bc5224dad14ff0f41f1d5

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

5e81335fix

comment:5 Changed 23 months ago by git

  • Commit changed from 5e81335087a4146e200bc5224dad14ff0f41f1d5 to ef06b0d60f1d184b8eb38ed5c212d0e3f96baf9d

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

ef06b0dtrac 27916 one more fix

comment:6 Changed 23 months ago by git

  • Commit changed from ef06b0d60f1d184b8eb38ed5c212d0e3f96baf9d to 2cfa48617c7af290330d950a846f7325a1283181

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 23 months ago by chapoton

  • Cc embray jdemeyer fbissey tscrim added

green bot, please review

comment:8 Changed 23 months ago by chapoton

  • Status changed from needs_work to needs_review

comment:9 Changed 23 months ago by chapoton

  • Cc jhpalmieri vklein added

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

comment:10 Changed 23 months ago by tscrim

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 23 months ago by 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 23 months ago by git

  • Commit changed from 2cfa48617c7af290330d950a846f7325a1283181 to 3c809588f7c5e7414629fdf1850c876019090eed

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 23 months ago by chapoton

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

comment:14 Changed 23 months ago by tscrim

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

comment:15 Changed 23 months ago by chapoton

  • Branch changed from u/chapoton/27916 to u/chapoton/27916_original
  • Commit changed from 3c809588f7c5e7414629fdf1850c876019090eed to 2cfa48617c7af290330d950a846f7325a1283181

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 23 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Then so be it.

comment:17 Changed 23 months ago by embray

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 23 months ago by 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 23 months ago by embray

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 23 months ago by embray

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 22 months ago by vbraun

  • Branch changed from u/chapoton/27916_original to 2cfa48617c7af290330d950a846f7325a1283181
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:22 Changed 22 months ago by embray

  • Milestone changed from sage-8.8 to sage-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.