Convert startup methods to context manager

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

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

[mkoeppe]

We now have a number of small, medium, and large distributions (see #29705 for distribution names in boldface, in particular on tickets that need review), so the question of how to organize startup could be investigated with concrete examples.

[git]

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

​9ed6d2c	Merge branch 'develop' of https://github.com/sagemath/sage into public/build/startupWarning
​87cba2d	Remove unnecessary code in doctest forker

[git]

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

​8421f6b	Fix test
​c34c014	Use three states to represent startup

[gh-tobiasdiez]

As far as I can see (please correct me if I'm wrong), the other (distribution) packages don't use sage.all and thus shouldn't be affected by the changes to the startup guard.

I've now introduced a third "startup state" (Uninitialized) that represents that sage is used without sage.all (or before sage.all is imported), e.g. in a modularized environment. This makes it possible to fix #32619 in a more ergonomic way.

From my point of view this is ready for review.

[git]

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

​339fd66

Fix typo

[mkoeppe]

This looks like a good solution. I'll take a closer look soon and test it with the modularized distributions

[gh-tobiasdiez]

Replying to mkoeppe:

This looks like a good solution. I'll take a closer look soon and test it with the modularized distributions

Did you already found the time to test it?

[mkoeppe]

I have been testing this for a while now with #32432, and there are no problems. (As you point out in comment:57, it's not really being invoked in this context). On the other hand, I don't have additional evidence yet that this change going in a useful direction for modularization.

[mkoeppe]

I confirm that it fixes the error shown by ./sage -t src/sage/all.py noted in the ticket description of #33531 (the error is replaced by 21 pytest warnings).

Tests pass.

[gh-tobiasdiez]

I think having an explicit start and endpoint for the resolution of the lazy imports is already an improvement as it converts the state management to a local thing, removing the "external" component.

Also the design should be flexible enough to incorporate new needs that may arise in the context of modularization.

[mkoeppe]

Replying to gh-tobiasdiez:

I think having an explicit start and endpoint for the resolution of the lazy imports is already an improvement

Yes, it's good that there's a start now.

[mkoeppe]

But I don't understand the semantics of startup. Why would it allow going from FINISHED back to RUNNING?

[gh-tobiasdiez]

That's needed due to the nature of how the doctests are invoked. At the beginning sage/all is imported, so the state is already "FINISHED". In order to write a meaningful test, one has to be able to reset this state (this is similar to the old test_fake_startup method).

[mkoeppe]

I think I would prefer it if this only-for-doctesting behavior was invoked explicitly. I think in normal operation, it should just remain in FINISHED.

[mkoeppe]

And actually I don't think startup_guard should be a new separate module.

[gh-tobiasdiez]

Replying to mkoeppe:

I think I would prefer it if this only-for-doctesting behavior was invoked explicitly. I think in normal operation, it should just remain in FINISHED.

I'm not sure, but what output would you expect from

with startup_guard.startup():
   print(startup_guard.startup_state)

print(startup_guard.startup_state)

with startup_guard.startup():
   print(startup_guard.startup_state)

Since this is now a proper guard, for me "RUNNING, FINISHED, RUNNING" feels correctly.

[gh-tobiasdiez]

Replying to mkoeppe:

And actually I don't think startup_guard should be a new separate module.

Where would you put it?

[mkoeppe]

Where it was - lazy_import.

[mkoeppe]

Replying to gh-tobiasdiez:

Since this is now a proper guard, for me "RUNNING, FINISHED, RUNNING" feels correctly.

It makes no sense to discuss this abstractly, without reference to what the guard protects.

[gh-tobiasdiez]

Replying to mkoeppe:

Where it was - lazy_import.

I guess these points are connected. For me the guard is a general "we in the progress of starting sage", which is then used by the lazy import feature to decide how imports during the startup are handled. But the startup guard could for example also used to control logging or other things.

[gh-tobiasdiez]

Replying to mkoeppe:

Replying to gh-tobiasdiez:

Since this is now a proper guard, for me "RUNNING, FINISHED, RUNNING" feels correctly.

It makes no sense to discuss this abstractly, without reference to what the guard protects.

That feels like an abstract answer (a bit of pun intended). Of course the startup guard signals that sage is starting (or if you want that lazy imports are lazy). So with that, what is the expected behavior of

with startup_guard.startup():
   lazy_import(xyz1)

lazy_import(xyz2)

with startup_guard.startup():
   lazy_import(xyz3)

I would say only xyz2 should be imported directly, while 1 and 3 are only imported lazily.

[mkoeppe]

Replying to gh-tobiasdiez:

I would say only xyz2 should be imported directly, while 1 and 3 are only imported lazily.

No, that's absolutely not what the intended semantics is. lazy imports are always lazy.

[mkoeppe]

Replying to gh-tobiasdiez:

For me the guard is a general "we in the progress of starting sage", which is then used by the lazy import feature to decide how imports during the startup are handled. But the startup guard could for example also used to control logging or other things.

I'd say this is speculative/premature abstraction. It's first necessary to capture the intended semantics for the actual use case.

[gh-tobiasdiez]

Replying to mkoeppe:

Replying to gh-tobiasdiez:

For me the guard is a general "we in the progress of starting sage", which is then used by the lazy import feature to decide how imports during the startup are handled. But the startup guard could for example also used to control logging or other things.

I'd say this is speculative/premature abstraction. It's first necessary to capture the intended semantics for the actual use case.

The code above was of course only meant schematically. Read "what should be imported directly" as "what is the output of _get_imports_resolved_at_startup".

[mkoeppe]

Well, the review says: Someone needs to figure out the semantics before pushing this code.

[git]

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

​998e9e6

Merge branch 'develop' into public/build/startupWarning

[git]

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

​5674655

Narrow down scope of lock

[gh-tobiasdiez]

I've narrowed down the scope of the lock/guard. What do you think?

[mkoeppe]

Thanks, this is better for the discussion of the needed semantics.

So what needs to be figured out is: What should happen if 1) There is a sequence of two start ups. 2) Two startups are nested.

[gh-tobiasdiez]

That feels like an academic issues since the actual code doesn't use either of these constructions and can easily be changed if these situations occur in the future.

[mkoeppe]

It's just hard to see what the improvement brought by the ticket would be. Replacing True/False? by LOCKED/UNLOCKED can't count as an improvement; and the semantics is made murkier, not clearer (see comment:69).

[mkoeppe]

Replying to mkoeppe:

What should happen if 1) There is a sequence of two start ups.

For example, if a user tries to reload the module sage.all.

[gh-tobiasdiez]

Replying to mkoeppe:

It's just hard to see what the improvement brought by the ticket would be. Replacing True/False? by LOCKED/UNLOCKED can't count as an improvement; and the semantics is made murkier, not clearer (see comment:69).

As written in the ticket description, the ticket fixes a bug with importing (using importlib) the sage all module.

And yes, replacing the boolean by an enum is an improvement by itself, ​https://stackoverflow.com/questions/4337942/using-enum-vs-boolean.

[git]

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

​cfd791c	Fix sage.all
​264aa53	Fix lazy_import as well

[mkoeppe]

comment:87 is still unaddressed. Besides, the new term "lock" suggests that the mechanism is related concurrency (it isn't). Introducing a new term does not help clarify the semantics.

[gh-tobiasdiez]

Replying to mkoeppe:

comment:87 is still unaddressed. Besides, the new term "lock" suggests that the mechanism is related concurrency (it isn't). Introducing a new term does not help clarify the semantics.

There is a doctest describing the behavior of two sequential locks, the behavior of nested locks is not further specified as we don't need it yet (but as with this simplest implementation the both locks share the global "lock" variable, the state is in "locked" until the nested lock exits and then switches to "unlocked").

I'm open for suggestions for how to improve the terminology. I thought that "locked" is pretty standard for a "guard", e.g. file locks. "Started"/"Not started" or "Initialized/Not? initialized" is confusing with how sequential locks work.

[mkoeppe]

Replying to gh-tobiasdiez:

I thought that "locked" is pretty standard for a "guard", e.g. file locks. "Started"/"Not started" or "Initialized/Not? initialized" is confusing with how sequential locks work.

Yes, file locks are locks. They protect against concurrent operations.

The start-up guard has nothing to do with concurrency. It just makes no sense to work with this analogy.

[mkoeppe]

In other words, still comment:74

[gh-tobiasdiez]

Replying to mkoeppe:

In other words, still comment:74

That concerned the more general version, now the purpose and scope of the lock should be clear.

[mkoeppe]

Well, it clearly isn't clear, as I said in comment:96. Try to explain it in the documentation or in the ticket.

[mkoeppe]

comment:74 is: You are trying to explain things in too much generality, based on an abstraction that you have come up with. But the abstraction does not capture the purpose of the startup guard mechanism: What exactly is the guard intended to protect? Short of explaining this, you are essentially trying to explain to me what a bit is.

[gh-tobiasdiez]

The purpose of the guard/lock/whatever you want to call it is the same as in the develop branch: when the guard is active, lazy imports should not be resolved. The only difference is that the guard has to be activated now explicitly.

[mkoeppe]

Replying to gh-tobiasdiez:

when the guard is active, lazy imports should not be resolved.

No, that's not what it does.

[gh-tobiasdiez]

Replying to mkoeppe:

Replying to gh-tobiasdiez:

when the guard is active, lazy imports should not be resolved.

No, that's not what it does.

So what is it doing then?

[mkoeppe]

You can see it in the source code of LazyImport._get_object - it does not change what is resolved when or anything. It only informs developers (by printing warnings) whether the laziness was effective or if the module was imported anyway.

[gh-tobiasdiez]

Yes, but isn't this a qualification of what "should" in "lazy import should not be resolved" means? Also from the ticket description: "convert runtime warning about resolved lazy imports to doctest."

Anyway, now that we agree on the terminology, do you still have questions about the serial behavior?

[mkoeppe]

Replying to gh-tobiasdiez:

Yes, but isn't this a qualification of what "should" in "lazy import should not be resolved" means?

OK, I now understand what you meant by "should".

[mkoeppe]

Replying to gh-tobiasdiez:

do you still have questions about the serial behavior?

Yes. You are changing the serial behavior in this ticket. Why is this change an improvement over what's in the develop branch?

[mkoeppe]

Replying to gh-tobiasdiez:

now that we agree on the terminology

We don't. It's not a lock, so please don't introduce this terminology in the ticket.

[gh-tobiasdiez]

Replying to mkoeppe:

Replying to gh-tobiasdiez:

do you still have questions about the serial behavior?

Yes. You are changing the serial behavior in this ticket. Why is this change an improvement over what's in the develop branch?

It's not really a change, the start/end methods were just called test_fake_startup/ensure_startup_finished. The improvement is currently only visible in the tests, as this is the only place where it is used. But I think it may be handy also in the all script to have more freedom on how to order/group imports there without being constrained by the lazy import guard.

[gh-tobiasdiez]

Replying to mkoeppe:

Replying to gh-tobiasdiez:

now that we agree on the terminology

We don't. It's not a lock, so please don't introduce this terminology in the ticket.

comment:95

[mkoeppe]

In comment:95 you called it a "sequential lock". It isn't.

[mkoeppe]

Or is your reference to comment:95 "I'm open for suggestions"?

Let's see. A "lock" is locked or unlocked. A "guard" is ... active/inactive? engaged/disengaged? on/off?

Anything like this sounds fine, I don't have a preference. It's just important to avoid using words that can be mistaken as standard technical terms when there is no relation.

[mkoeppe]

Replying to gh-tobiasdiez:

Replying to mkoeppe:

Replying to gh-tobiasdiez:

do you still have questions about the serial behavior?

Yes. You are changing the serial behavior in this ticket. Why is this change an improvement over what's in the develop branch?

It's not really a change, the start/end methods were just called test_fake_startup/ensure_startup_finished.

OK, that's not quite where I wanted to take the discussion.

Let's please ignore the old doctesting-only helper functions - they have no relevance for discussing the semantics.

In develop, finish_startup can only be called once, and an error is raised otherwise; and then there is ensure_startup_finished, which can be called multiple times and which is a no-op if it was called before.

[...] I think it may be handy also in the all script to have more freedom on how to order/group imports there without being constrained by the lazy import guard.

Yes, I am interested in this -- which is why I brought up the scenarios of sequences of such startup-guarded blocks and also nested startup-guarded blocks in comment:87.

So with the changes on this ticket, it becomes possible to have a sequence of two startup-guarded blocks:

with ...:
    Block 1
with ...:
    Block 2

I think we both agree on what the first block should do. Now, why should the second one behave as you implemented?

[gh-tobiasdiez]

Because its not important in which block the lazy import is placed, it's only important whether it's guarded or not. What other implementation would you suggest?

[mkoeppe]

Replying to gh-tobiasdiez:

Because its not important in which block the lazy import is placed, it's only important whether it's guarded or not.

But *why* do you think that this the right semantics?

[gh-tobiasdiez]

It seemed the most logical and easy to implement solution, that worked well with the currently only use case of the tests. What other implementation do you have in mind? Make the second block a noop?

[mkoeppe]

Replying to gh-tobiasdiez:

Make the second block a noop?

Yes, a kind of no-op is what I had in mind.

In the current code, this structure does not appear lexically (and I don't have a use case for it in modularization):

with ...:
    Block 1
with ...:
    Block 2

However, this sequence will be executed when a user reloads the sage.all module using importlib.reload. (Currently this is an error. And of course it is unclear if reloading actually can be made to work properly, given that we have so much complicated global state.)

So in this situation, in the second block, it would makes no sense to do any of the tracking of resolved lazy imports and emitting warnings. The startup was done already.

[mkoeppe]

On the other hand, this structure:

with ...:
    with ...:
        Block 1
    with ...:
        Block 2
    Block 3

would appear in the course of modularization in some follow-up ticket of #29941, #28925. Blocks 1 and 2 would be imports done by modules like sage.all__sagemath_environment, sage.all__sagemath_objects.

[gh-tobiasdiez]

Replying to mkoeppe:

Replying to gh-tobiasdiez:

Make the second block a noop?

Yes, a kind of no-op is what I had in mind.

In the current code, this structure does not appear lexically (and I don't have a use case for it in modularization):

with ...:
    Block 1
with ...:
    Block 2

However, this sequence will be executed when a user reloads the sage.all module using importlib.reload. (Currently this is an error. And of course it is unclear if reloading actually can be made to work properly, given that we have so much complicated global state.)

So in this situation, in the second block, it would makes no sense to do any of the tracking of resolved lazy imports and emitting warnings. The startup was done already.

In this use case the current implementation is almost noop, since now the guard is only checked in the doctests and no warnings are issued.

[gh-tobiasdiez]

Replying to mkoeppe:

On the other hand, this structure:

with ...:
    with ...:
        Block 1
    with ...:
        Block 2
    Block 3

would appear in the course of modularization in some follow-up ticket of #29941, #28925. Blocks 1 and 2 would be imports done by modules like sage.all__sagemath_environment, sage.all__sagemath_objects.

In my opinion, the submodules shouldn't have their own startup scripts. The all script is one of the things that make sage so hard to use as a library. But if the nested structure does arise in the future, then the implementation of the guard can easily be changed to accommodate of this.

[mkoeppe]

Replying to gh-tobiasdiez:

Replying to mkoeppe:

imports done by modules like sage.all__sagemath_environment, sage.all__sagemath_objects.

In my opinion, the submodules shouldn't have their own startup scripts. The all script is one of the things that make sage so hard to use as a library.

The all scripts have two purposes: (1) To orchestrate startup, and I agree with you that the library should be changed to make it unnecessary - that's #33580. (2) To fill the top-level namespace that is available at the sage: prompt. And this is not going away.

[mkoeppe]

Replying to gh-tobiasdiez:

But if the nested structure does arise in the future

Well, it will, as I said in comment:118.

then the implementation of the guard can easily be changed to accommodate of this.

In your current branch you are introducing a public interface (including new terminology) for the GuardState. That's not a good way forward when we already know that it does not capture what is needed.

[mkoeppe]

Replying to gh-tobiasdiez:

Replying to mkoeppe:

Replying to gh-tobiasdiez:

Make the second block a noop?

Yes, a kind of no-op is what I had in mind. [...] this sequence will be executed when a user reloads the sage.all module using importlib.reload. [...]

So in this situation, in the second block, it would makes no sense to do any of the tracking of resolved lazy imports and emitting warnings. The startup was done already.

In this use case the current implementation is almost noop, since now the guard is only checked in the doctests and no warnings are issued.

Not no-op enough. The doctests can also be invoked from within a running Sage.

[gh-tobiasdiez]

Replying to mkoeppe:

(2) To fill the top-level namespace that is available at the sage: prompt. And this is not going away.

But you only have one prompt so one global guard should be sufficient. I don't see the need why the submodule needs its own guard, or even why it's the responsibility of the module to declare what methods should be available in the sage prompt in the first place.

[gh-tobiasdiez]

Replying to mkoeppe:

Replying to gh-tobiasdiez:

Replying to mkoeppe:

Replying to gh-tobiasdiez:

Make the second block a noop?

Yes, a kind of no-op is what I had in mind. [...] this sequence will be executed when a user reloads the sage.all module using importlib.reload. [...]

So in this situation, in the second block, it would makes no sense to do any of the tracking of resolved lazy imports and emitting warnings. The startup was done already.

In this use case the current implementation is almost noop, since now the guard is only checked in the doctests and no warnings are issued.

Not no-op enough. The doctests can also be invoked from within a running Sage.

The test would still only fail if there are already resolved lazy imports in the first load.

[mkoeppe]

Replying to gh-tobiasdiez:

Replying to mkoeppe:

(2) To fill the top-level namespace that is available at the sage: prompt. And this is not going away.

But you only have one prompt

Nope. See ​https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#testing-distribution-packages

I don't see the need why the submodule needs its own guard, or even why it's the responsibility of the module

Not the module, the distribution.

[mkoeppe]

Replying to gh-tobiasdiez:

Replying to mkoeppe:

Not no-op enough. The doctests can also be invoked from within a running Sage.

The test would still only fail if there are already resolved lazy imports in the first load.

There are two types warnings, right? Let's break the analysis down by type.