Opened 12 months ago

Last modified 2 months ago

#30748 needs_review enhancement

Convert startup methods to context manager

Reported by: gh-tobiasdiez Owned by:
Priority: major Milestone: sage-9.5
Component: build Keywords: sd111
Cc: mkoeppe, chapoton, tscrim Merged in:
Authors: Tobias Diez Reviewers:
Report Upstream: N/A Work issues:
Branch: public/build/startupWarning (Commits, GitHub, GitLab) Commit: 7647fc2b97359689c059fb3408d06710948c4f1e
Dependencies: #31080 Stopgaps:

Status badges

Description (last modified by gh-tobiasdiez)

Convert startup methods to context manager, and convert runtime warning about resolved lazy imports to doctest.

Change History (52)

comment:1 Changed 12 months ago by gh-tobiasdiez

  • Status changed from new to needs_review

comment:2 Changed 12 months ago by mkoeppe

Perhaps the start_startup....finish_startup could be a context manager?

comment:3 Changed 12 months ago by gh-tobiasdiez

I like the idea! Where is the startup code located? I couldn't find the global "import sage.all".

Second question, is how LazyImport? gets access to the global startup guard. Should this be done via global?

comment:4 Changed 12 months ago by git

  • Commit changed from 99ae00c2ecb1ffcb8895697f34861f05dea840e6 to 11882e531bc80f6397f00d6a5a095b97a9e93804

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

11882e5Use context manager

comment:5 Changed 12 months ago by gh-tobiasdiez

I've now implemented the idea with the context manager.

comment:6 Changed 11 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:7 Changed 11 months ago by git

  • Commit changed from 11882e531bc80f6397f00d6a5a095b97a9e93804 to 6a52fbf72d83b2488845f4caeb15798ed5a217e7

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

6a52fbfRemove lazy import finish startup

comment:8 Changed 11 months ago by mkoeppe

  • Status changed from needs_review to needs_work

Doctests fail (are not in the correct format), see patchbot output

comment:9 Changed 11 months ago by git

  • Commit changed from 6a52fbf72d83b2488845f4caeb15798ed5a217e7 to b952bb5661206b5614dfe6117c27021627b19616

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

f96025eMerge branch 'develop' of git://github.com/sagemath/sage into public/build/startupWarning
b952bb5Fix doctests

comment:10 Changed 11 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

Should be fixed now.

Last edited 11 months ago by gh-tobiasdiez (previous) (diff)

comment:11 follow-up: Changed 11 months ago by mkoeppe

Patchbot still complains

comment:12 Changed 11 months ago by gh-tobiasdiez

The pyflakes warnings are not related to my changes, and the additional startup_module is also expected since startup_guard is now a new class. So I'm not sure what I can/should do.

comment:13 in reply to: ↑ 11 Changed 11 months ago by mkoeppe

  • Status changed from needs_review to needs_work

Replying to mkoeppe:

Patchbot still complains

... take a look at "shortlog" and you will see many errors.

comment:14 Changed 11 months ago by git

  • Commit changed from b952bb5661206b5614dfe6117c27021627b19616 to c6857ddffa15533845133867febbad1b7f854d4d

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

c6857ddHopefully fix doctest

comment:15 Changed 11 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

Ah thanks, wasn't aware of these links before. Should be fixed now (hopefully).

comment:16 Changed 11 months ago by mkoeppe

  • Status changed from needs_review to needs_work

please set back to needs review when the doctests succeed

comment:17 Changed 11 months ago by gh-tobiasdiez

Sorry, I thought I've fixed all doctests.

On a closer inspection, now a lot of doctests are failing because of this warnings print('Option at_startup=True for lazy import {0} not needed anymore'.format(self._name)). I have to admit I don't really understand the purpose of this message, but I guess it should be shown if lazy_import is used with at_startup = true but sage is actually not loading at this point. If that's the case, then the current output of these doctests seems to be correct now, but I'm not sure why these warnings were not shown before.

comment:18 Changed 11 months ago by git

  • Commit changed from c6857ddffa15533845133867febbad1b7f854d4d to 23da14b03ee782934119597465f8ab1f9db5318f

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

23da14bWrap each sage.all import in startup guard

comment:19 Changed 11 months ago by git

  • Commit changed from 23da14b03ee782934119597465f8ab1f9db5318f to 2f4055e37bea3705a2ab165da42c32363801ddae

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

2f4055eSpecify that exit was succesful

comment:20 Changed 11 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

comment:21 Changed 11 months ago by git

  • Commit changed from 2f4055e37bea3705a2ab165da42c32363801ddae to d4d94a8dec4d876517974bc94de3b83ede14061f

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

d4d94a8Indeed replace exception by print statement

comment:22 Changed 10 months ago by gh-tobiasdiez

  • Cc chapoton added

I would appreciate if this could be reviewed relatively soon, as it's blocking my local development (I've to include it in every branch I'm working on). Thanks!

comment:23 Changed 10 months ago by mkoeppe

  • Cc tscrim added

comment:24 Changed 10 months ago by mkoeppe

  • Keywords sd111 added

Hoping we can make progress on this ticket this week - https://wiki.sagemath.org/days111

comment:25 Changed 9 months ago by gh-tobiasdiez

Did you had a chance to look at this ticket during the Sage days?

comment:26 Changed 9 months ago by mkoeppe

Yes, briefly. Overall I'm not convinced by the change to a context manager (which I had suggested). So let's please split this ticket please into a minimal change (error to warning) and defer the change to using a context manager for later consideration.

comment:27 follow-up: Changed 9 months ago by gh-tobiasdiez

  • Dependencies set to #31080
  • Description modified (diff)

Ok, I've extracted it to #31080. But I would still ask you to also review this ticket, as otherwise I have to subtract it from some of my other tickets (which is not as easy as just pulling the new version of a branch).

Why don't you like the context manager? I think it's the right tool, as we are managing a local state.

comment:28 Changed 9 months ago by gh-tobiasdiez

  • Summary changed from Replace startup exception by warning to Convert startup methods to context manager

comment:29 Changed 9 months ago by mkoeppe

Making changes always has a cost, and the benefit here is not clear.

We only have a small number of developers who know about the details of the issues with circular imports in sage. Changing code makes it harder for them to contribute later.

comment:30 in reply to: ↑ 27 Changed 9 months ago by mkoeppe

Replying to gh-tobiasdiez:

Why don't you like the context manager? I think it's the right tool, as we are managing a local state.

I also thought it could be the right tool -- but I think it's best to revisit it in the context of the modularization of the core of sagelib (tickets such as #29865)

comment:31 Changed 9 months ago by git

  • Commit changed from d4d94a8dec4d876517974bc94de3b83ede14061f to a462713ac1fcb11cee4df5ff845615013c2b2355

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

a462713Verify that no imports are resolved by doctest

comment:32 Changed 9 months ago by gh-tobiasdiez

I've thought about this again and realized that one actually would like to neither throw an error nor print an error message. Instead one should try to catch the resolved import already during development time. Thus, I've now implemented it as a doctest that fails as soon as there are resolved imports during startup.

What do you think?

comment:33 Changed 9 months ago by mkoeppe

I think I agree but this is orthogonal to the conversion to a context manager -- so shouldn't this be done in #31080 instead?

comment:34 Changed 9 months ago by git

  • Commit changed from a462713ac1fcb11cee4df5ff845615013c2b2355 to 1b597a0e76511747c26ab02a39fe12eb9159fd14

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

1b597a0Fix syntax

comment:35 follow-up: Changed 9 months ago by gh-tobiasdiez

I needed the context manager to write the tests.

but I think it's best to revisit it in the context of the modularization of the core of sagelib (tickets such as #29865)

What does the context manager has to do with modularization? It sets only the IS_STARTUP flag...

comment:36 Changed 9 months ago by mkoeppe

How is the startup phase defined?

comment:37 follow-up: Changed 9 months ago by gh-tobiasdiez

It's essentially equivalent to "during the import of sage.all", but not quite. There are a few imports in sage.all that are are outside of the startup phase (and I didn't change anything there). The new context manger makes this quite clear: startup phase = everything between enter/exit of the startup_guard (or, equivalently, within the with startup_guard block).

comment:38 Changed 9 months ago by git

  • Commit changed from 1b597a0e76511747c26ab02a39fe12eb9159fd14 to c581af7263b2f5164d1e613e368dc6612f638c33

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

c581af7Fix syntax error

comment:39 in reply to: ↑ 37 Changed 9 months ago by mkoeppe

Replying to gh-tobiasdiez:

It's essentially equivalent to "during the import of sage.all", but not quite. There are a few imports in sage.all that are are outside of the startup phase (and I didn't change anything there). The new context manger makes this quite clear: startup phase = everything between enter/exit of the startup_guard (or, equivalently, within the with startup_guard block).

Well, the concept of sage.all will certainly be affected by the modularization...

comment:40 Changed 9 months ago by gh-tobiasdiez

Sure, but in a modularized setting you probably don't need this check since it's only purpose right now is to make sure that lazy imports are indeed lazy and not already resolved during startup. If that's the case for sage.all, then it's also the case for sub-modules.

In either case, the startup context manager will make it easy to define also other parts as startup in a modularized setting in case this is necessary.

comment:41 Changed 9 months ago by mkoeppe

Without code that's a little bit too vague, which is why it's too early to do this ticket. In particular, it's unclear whether a single flag / context manager can really do the job for several modules.

comment:42 Changed 9 months ago by gh-tobiasdiez

I agree, but the new code is still an improvement over the current one in my opinion (which used a custom-build context manager in some sense). So can we please not hold this ticket off, just because there might be possible changes in the future...

Moreover, the startup manager can easily be extended to handle multiple startup contexts by passing the context name as an argument to the startup method, including neasting etc because we use a context manager (which is not easily possible with the current code).

Last edited 9 months ago by gh-tobiasdiez (previous) (diff)

comment:43 in reply to: ↑ 35 ; follow-up: Changed 9 months ago by mkoeppe

Replying to gh-tobiasdiez:

I needed the context manager to write the tests.

I don't think so, the same thing can be done in the original code

comment:44 follow-up: Changed 9 months ago by mkoeppe

By the way, I would guess that also the print message

   print(f"Option ``at_startup=True`` for lazy import {self._name} not needed anymore"`

should be handled in the doctest as well instead of printing at runtime.

comment:45 in reply to: ↑ 43 Changed 9 months ago by gh-tobiasdiez

Replying to mkoeppe:

Replying to gh-tobiasdiez:

I needed the context manager to write the tests.

I don't think so, the same thing can be done in the original code

I was not able to, but you are invited to try (although its more time effective to just review this ticket...).

comment:46 in reply to: ↑ 44 Changed 9 months ago by gh-tobiasdiez

Replying to mkoeppe:

By the way, I would guess that also the print message

   print(f"Option ``at_startup=True`` for lazy import {self._name} not needed anymore"`

should be handled in the doctest as well instead of printing at runtime.

Good point, something for a followup ticket.

comment:47 Changed 9 months ago by git

  • Commit changed from c581af7263b2f5164d1e613e368dc6612f638c33 to 7647fc2b97359689c059fb3408d06710948c4f1e

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

7647fc2Merge branch 'develop' of git://github.com/sagemath/sage into public/build/startupWarning

comment:48 Changed 9 months ago by gh-tobiasdiez

  • Description modified (diff)

comment:49 Changed 9 months ago by gh-tobiasdiez

How should we proceed with this ticket?

comment:50 Changed 9 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

As previously suggested, we defer it until the design constraints from modularization become clearer.

comment:51 Changed 4 months ago by gh-tobiasdiez

What's the status of the modularization? Any new input that is relevant for this PR?

comment:52 Changed 2 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

Setting a new milestone for this ticket based on a cursory review.

Note: See TracTickets for help on using tickets.