Opened 22 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.7
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: 264aa53e8b47dd5ebb3b53d26f8f47c8ff2d5beb
Dependencies: #31080 Stopgaps:

Status badges

Description (last modified by mkoeppe)

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

Change History (127)

comment:1 Changed 22 months ago by gh-tobiasdiez

  • Status changed from new to needs_review

comment:2 Changed 22 months ago by mkoeppe

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

comment:3 Changed 22 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 22 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 22 months ago by gh-tobiasdiez

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

comment:6 Changed 22 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:7 Changed 22 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 21 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 21 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 21 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

Should be fixed now.

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

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

Patchbot still complains

comment:12 Changed 21 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 21 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 21 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 21 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 21 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 21 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 21 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 21 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 21 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

comment:21 Changed 21 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 20 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 20 months ago by mkoeppe

  • Cc tscrim added

comment:24 Changed 20 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 20 months ago by gh-tobiasdiez

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

comment:26 Changed 20 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 20 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 20 months ago by gh-tobiasdiez

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

comment:29 Changed 20 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 20 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 20 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 20 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 20 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 20 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 20 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 20 months ago by mkoeppe

How is the startup phase defined?

comment:37 follow-up: Changed 20 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 20 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 20 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 20 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 20 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 20 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 20 months ago by gh-tobiasdiez (previous) (diff)

comment:43 in reply to: ↑ 35 ; follow-up: Changed 20 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 20 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 20 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 20 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 20 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 20 months ago by gh-tobiasdiez

  • Description modified (diff)

comment:49 Changed 19 months ago by gh-tobiasdiez

How should we proceed with this ticket?

comment:50 Changed 19 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 15 months ago by gh-tobiasdiez

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

comment:52 Changed 13 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.

comment:53 Changed 8 months ago by mjo

  • Status changed from needs_review to needs_info

comment:54 Changed 8 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

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.

comment:55 Changed 8 months ago by git

  • Commit changed from 7647fc2b97359689c059fb3408d06710948c4f1e to 87cba2d211c112741350d60a4d495b1de02f5c97

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

9ed6d2cMerge branch 'develop' of https://github.com/sagemath/sage into public/build/startupWarning
87cba2dRemove unnecessary code in doctest forker

comment:56 Changed 8 months ago by git

  • Commit changed from 87cba2d211c112741350d60a4d495b1de02f5c97 to c34c014645f04987796df7134c158dd8a5b05486

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

8421f6bFix test
c34c014Use three states to represent startup

comment:57 Changed 8 months ago by gh-tobiasdiez

  • Status changed from needs_info to needs_review

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.

comment:58 Changed 8 months ago by git

  • Commit changed from c34c014645f04987796df7134c158dd8a5b05486 to 339fd660b7d6dfefb9527bc524ea9c71aa57465e

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

339fd66Fix typo

comment:59 follow-up: Changed 8 months ago by mkoeppe

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

comment:60 in reply to: ↑ 59 Changed 7 months ago by 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?

comment:61 Changed 5 months ago by gh-tobiasdiez

  • Description modified (diff)

comment:62 Changed 5 months ago by 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.

comment:63 Changed 5 months ago by mkoeppe

  • Description modified (diff)

comment:64 Changed 5 months ago by 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.

comment:65 follow-up: Changed 5 months ago by 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.

comment:66 in reply to: ↑ 65 Changed 5 months ago by 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.

comment:67 Changed 5 months ago by mkoeppe

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

comment:68 Changed 5 months ago by 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).

comment:69 follow-up: Changed 5 months ago by 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.

comment:70 follow-up: Changed 5 months ago by mkoeppe

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

comment:71 in reply to: ↑ 69 ; follow-up: Changed 5 months ago by 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.

comment:72 in reply to: ↑ 70 Changed 5 months ago by gh-tobiasdiez

Replying to mkoeppe:

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

Where would you put it?

comment:73 follow-up: Changed 5 months ago by mkoeppe

Where it was - lazy_import.

comment:74 in reply to: ↑ 71 ; follow-up: Changed 5 months ago by 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.

comment:75 in reply to: ↑ 73 ; follow-ups: Changed 5 months ago by 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.

comment:76 in reply to: ↑ 74 ; follow-up: Changed 5 months ago by 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.

comment:77 in reply to: ↑ 76 Changed 5 months ago by 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.

comment:78 in reply to: ↑ 75 Changed 5 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:79 in reply to: ↑ 75 ; follow-up: Changed 5 months ago by 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.

comment:80 in reply to: ↑ 79 Changed 5 months ago by 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".

comment:81 Changed 4 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

comment:82 Changed 4 months ago by mkoeppe

  • Status changed from needs_review to needs_work

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

comment:83 Changed 4 months ago by mkoeppe

  • Milestone changed from sage-9.6 to sage-9.7

comment:84 Changed 4 months ago by git

  • Commit changed from 339fd660b7d6dfefb9527bc524ea9c71aa57465e to 998e9e62af2a25792d0450af0a7f23cc7f4225a1

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

998e9e6Merge branch 'develop' into public/build/startupWarning

comment:85 Changed 4 months ago by git

  • Commit changed from 998e9e62af2a25792d0450af0a7f23cc7f4225a1 to 567465572d55907dae5f790385e3663c64d3c838

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

5674655Narrow down scope of lock

comment:86 Changed 4 months ago by gh-tobiasdiez

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

comment:87 follow-up: Changed 4 months ago by 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.

comment:88 Changed 4 months ago by 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.

comment:89 follow-up: Changed 4 months ago by 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).

comment:90 in reply to: ↑ 87 Changed 4 months ago by 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.

comment:91 in reply to: ↑ 89 Changed 4 months ago by 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.

comment:92 Changed 4 months ago by git

  • Commit changed from 567465572d55907dae5f790385e3663c64d3c838 to 264aa53e8b47dd5ebb3b53d26f8f47c8ff2d5beb

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

cfd791cFix sage.all
264aa53Fix lazy_import as well

comment:93 Changed 3 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

comment:94 follow-up: Changed 3 months ago by mkoeppe

  • Status changed from needs_review to needs_work

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.

comment:95 in reply to: ↑ 94 ; follow-up: Changed 3 months ago by gh-tobiasdiez

  • Status changed from needs_work to needs_review

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.

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

comment:96 in reply to: ↑ 95 Changed 3 months ago by 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.

comment:97 follow-up: Changed 3 months ago by mkoeppe

In other words, still comment:74

comment:98 in reply to: ↑ 97 ; follow-up: Changed 3 months ago by 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.

comment:99 in reply to: ↑ 98 Changed 3 months ago by mkoeppe

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

comment:100 Changed 3 months ago by 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.

comment:101 follow-up: Changed 3 months ago by 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.

comment:102 in reply to: ↑ 101 ; follow-up: Changed 3 months ago by mkoeppe

Replying to gh-tobiasdiez:

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

No, that's not what it does.

comment:103 in reply to: ↑ 102 Changed 3 months ago by 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?

comment:104 Changed 3 months ago by 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.

comment:105 follow-ups: Changed 3 months ago by 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?

comment:106 in reply to: ↑ 105 Changed 3 months ago by 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".

comment:107 in reply to: ↑ 105 ; follow-up: Changed 3 months ago by 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?

comment:108 in reply to: ↑ 105 ; follow-up: Changed 3 months ago by 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:109 in reply to: ↑ 107 ; follow-up: Changed 2 months ago by 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.

comment:110 in reply to: ↑ 108 Changed 2 months ago by 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

comment:111 Changed 2 months ago by mkoeppe

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

comment:112 Changed 2 months ago by 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.

comment:113 in reply to: ↑ 109 Changed 2 months ago by 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?

comment:114 follow-up: Changed 2 months ago by 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?

comment:115 in reply to: ↑ 114 Changed 2 months ago by 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?

comment:116 follow-up: Changed 2 months ago by 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?

comment:117 in reply to: ↑ 116 ; follow-up: Changed 2 months ago by 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.

Last edited 2 months ago by mkoeppe (previous) (diff)

comment:118 follow-up: Changed 2 months ago by 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.

Last edited 2 months ago by mkoeppe (previous) (diff)

comment:119 in reply to: ↑ 117 ; follow-up: Changed 2 months ago by 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.

comment:120 in reply to: ↑ 118 ; follow-ups: Changed 2 months ago by 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.

comment:121 in reply to: ↑ 120 ; follow-up: Changed 2 months ago by 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.

comment:122 in reply to: ↑ 120 Changed 2 months ago by 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.

comment:123 in reply to: ↑ 119 ; follow-up: Changed 2 months ago by 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.

comment:124 in reply to: ↑ 121 ; follow-up: Changed 2 months ago by 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.

comment:125 in reply to: ↑ 123 ; follow-up: Changed 2 months ago by 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.

comment:126 in reply to: ↑ 124 Changed 2 months ago by 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.

comment:127 in reply to: ↑ 125 Changed 2 months ago by 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.

Note: See TracTickets for help on using tickets.