Convert startup methods to context manager

[gh-tobiasdiez]

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

[mkoeppe]

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

[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?

[git]

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

​11882e5

Use context manager

[gh-tobiasdiez]

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

[git]

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

​6a52fbf

Remove lazy import finish startup

[mkoeppe]

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

[git]

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

[gh-tobiasdiez]

Should be fixed now.

[mkoeppe]

Patchbot still complains

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

[mkoeppe]

Replying to mkoeppe:

Patchbot still complains

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

[git]

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

​c6857dd

Hopefully fix doctest

[gh-tobiasdiez]

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

[mkoeppe]

please set back to needs review when the doctests succeed

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

[git]

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

​23da14b

Wrap each sage.all import in startup guard

[git]

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

​2f4055e

Specify that exit was succesful

[git]

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

​d4d94a8

Indeed replace exception by print statement

[gh-tobiasdiez]

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]

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

[gh-tobiasdiez]

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

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

[gh-tobiasdiez]

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.

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

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

[git]

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

​a462713

Verify that no imports are resolved by doctest

[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?

[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?

[git]

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

​1b597a0

Fix syntax

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

[mkoeppe]

How is the startup phase defined?

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

[git]

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

​c581af7

Fix syntax error

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

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

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

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

[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

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

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

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

[git]

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

​7647fc2

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

[gh-tobiasdiez]

How should we proceed with this ticket?

[mkoeppe]

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

[gh-tobiasdiez]

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

[mkoeppe]

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