Convert startup methods to context manager

[tobiasdiez]

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

This fixes a bug that prevents importing the main all module (e.g. in the context of pytest - see #33531).

Depends on #31080

CC: @mkoeppe @fchapoton @tscrim

Component: build

Keywords: sd111

Author: Tobias Diez

Branch/Commit: public/build/startupWarning @ 264aa53

Issue created by migration from https://trac.sagemath.org/ticket/30748

[mkoeppe]

comment:2

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

[tobiasdiez]

comment:3

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?

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

11882e5

Use context manager

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 99ae00c to 11882e5

[tobiasdiez]

comment:5

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

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 11882e5 to 6a52fbf

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

6a52fbf

Remove lazy import finish startup

[mkoeppe]

comment:8

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

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 6a52fbf to b952bb5

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

f96025e	Merge branch 'develop' of git://github.com/sagemath/sage into public/build/startupWarning
b952bb5	Fix doctests

[tobiasdiez]

comment:10

Should be fixed now.

[mkoeppe]

comment:11

Patchbot still complains

[tobiasdiez]

comment:12

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.

[mkoeppe]

comment:13

Replying to @mkoeppe:

Patchbot still complains

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

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from b952bb5 to c6857dd

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

c6857dd

Hopefully fix doctest

[tobiasdiez]

comment:15

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

[mkoeppe]

comment:16

please set back to needs review when the doctests succeed

[tobiasdiez]

comment:17

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.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from c6857dd to 23da14b

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

23da14b

Wrap each sage.all import in startup guard

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

2f4055e

Specify that exit was succesful

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 23da14b to 2f4055e

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

d4d94a8

Indeed replace exception by print statement

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 2f4055e to d4d94a8

[tobiasdiez]

comment:22

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!

[mkoeppe]

Changed keywords from none to sd111

[mkoeppe]

comment:24

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

[tobiasdiez]

comment:25

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

[mkoeppe]

comment:26

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.

[tobiasdiez]

comment:27

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.

[tobiasdiez]

Description changed:

--- 
+++ 
@@ -1,3 +1 @@
-In #22752 it was introduced that when a lazy import is used during startup, then an RuntimeException is thrown. This leads to problems if you want to use code with lazy imports in a library mode (e.g. in standalone tests or python scripts). To support these use cases, I've degraded the exception to a print statement.
-
-Another possible solution would be to also introduce a `start_startup` method that is used at the very beginning of loading sage, and then only consider everything between `start_startup` and `finish_startup` as "startup".
+Convert startup methods to context manager.

[tobiasdiez]

Dependencies: #31080

[mkoeppe]

comment:29

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.

[mkoeppe]

comment:30

Replying to @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)

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

a462713

Verify that no imports are resolved by doctest

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from d4d94a8 to a462713

[tobiasdiez]

comment:32

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?

[mkoeppe]

comment:33

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

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from a462713 to 1b597a0

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

1b597a0

Fix syntax

[tobiasdiez]

comment:35

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...

[mkoeppe]

comment:36

How is the startup phase defined?

[tobiasdiez]

comment:37

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).

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 1b597a0 to c581af7

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

c581af7

Fix syntax error

[mkoeppe]

comment:39

Replying to @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...

[tobiasdiez]

comment:40

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.

[mkoeppe]

comment:41

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.

[tobiasdiez]

comment:42

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).