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: |
Description (last modified by )
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
- Status changed from new to needs_review
comment:2 Changed 22 months ago by
comment:3 Changed 22 months ago by
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
- Commit changed from 99ae00c2ecb1ffcb8895697f34861f05dea840e6 to 11882e531bc80f6397f00d6a5a095b97a9e93804
Branch pushed to git repo; I updated commit sha1. New commits:
11882e5 | Use context manager
|
comment:5 Changed 22 months ago by
I've now implemented the idea with the context manager.
comment:6 Changed 22 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:7 Changed 22 months ago by
- Commit changed from 11882e531bc80f6397f00d6a5a095b97a9e93804 to 6a52fbf72d83b2488845f4caeb15798ed5a217e7
Branch pushed to git repo; I updated commit sha1. New commits:
6a52fbf | Remove lazy import finish startup
|
comment:8 Changed 21 months ago by
- 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
- Commit changed from 6a52fbf72d83b2488845f4caeb15798ed5a217e7 to b952bb5661206b5614dfe6117c27021627b19616
comment:10 Changed 21 months ago by
- Status changed from needs_work to needs_review
Should be fixed now.
comment:11 follow-up: ↓ 13 Changed 21 months ago by
Patchbot still complains
comment:12 Changed 21 months ago by
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
- 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
- Commit changed from b952bb5661206b5614dfe6117c27021627b19616 to c6857ddffa15533845133867febbad1b7f854d4d
Branch pushed to git repo; I updated commit sha1. New commits:
c6857dd | Hopefully fix doctest
|
comment:15 Changed 21 months ago by
- 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
- 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
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
- Commit changed from c6857ddffa15533845133867febbad1b7f854d4d to 23da14b03ee782934119597465f8ab1f9db5318f
Branch pushed to git repo; I updated commit sha1. New commits:
23da14b | Wrap each sage.all import in startup guard
|
comment:19 Changed 21 months ago by
- Commit changed from 23da14b03ee782934119597465f8ab1f9db5318f to 2f4055e37bea3705a2ab165da42c32363801ddae
Branch pushed to git repo; I updated commit sha1. New commits:
2f4055e | Specify that exit was succesful
|
comment:20 Changed 21 months ago by
- Status changed from needs_work to needs_review
comment:21 Changed 21 months ago by
- Commit changed from 2f4055e37bea3705a2ab165da42c32363801ddae to d4d94a8dec4d876517974bc94de3b83ede14061f
Branch pushed to git repo; I updated commit sha1. New commits:
d4d94a8 | Indeed replace exception by print statement
|
comment:22 Changed 20 months ago by
- 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
- Cc tscrim added
comment:24 Changed 20 months ago by
- 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
Did you had a chance to look at this ticket during the Sage days?
comment:26 Changed 20 months ago by
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: ↓ 30 Changed 20 months ago by
- 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
- Summary changed from Replace startup exception by warning to Convert startup methods to context manager
comment:29 Changed 20 months ago by
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
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
- Commit changed from d4d94a8dec4d876517974bc94de3b83ede14061f to a462713ac1fcb11cee4df5ff845615013c2b2355
Branch pushed to git repo; I updated commit sha1. New commits:
a462713 | Verify that no imports are resolved by doctest
|
comment:32 Changed 20 months ago by
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
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
- Commit changed from a462713ac1fcb11cee4df5ff845615013c2b2355 to 1b597a0e76511747c26ab02a39fe12eb9159fd14
Branch pushed to git repo; I updated commit sha1. New commits:
1b597a0 | Fix syntax
|
comment:35 follow-up: ↓ 43 Changed 20 months ago by
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
How is the startup phase defined?
comment:37 follow-up: ↓ 39 Changed 20 months ago by
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
- Commit changed from 1b597a0e76511747c26ab02a39fe12eb9159fd14 to c581af7263b2f5164d1e613e368dc6612f638c33
Branch pushed to git repo; I updated commit sha1. New commits:
c581af7 | Fix syntax error
|
comment:39 in reply to: ↑ 37 Changed 20 months ago by
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
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
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
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).
comment:43 in reply to: ↑ 35 ; follow-up: ↓ 45 Changed 20 months ago by
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: ↓ 46 Changed 20 months ago by
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
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
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
- Commit changed from c581af7263b2f5164d1e613e368dc6612f638c33 to 7647fc2b97359689c059fb3408d06710948c4f1e
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
|
comment:48 Changed 20 months ago by
- Description modified (diff)
comment:49 Changed 19 months ago by
How should we proceed with this ticket?
comment:50 Changed 19 months ago by
- 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
What's the status of the modularization? Any new input that is relevant for this PR?
comment:52 Changed 13 months ago by
- 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
- Status changed from needs_review to needs_info
comment:54 Changed 8 months ago by
- 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
- Commit changed from 7647fc2b97359689c059fb3408d06710948c4f1e to 87cba2d211c112741350d60a4d495b1de02f5c97
comment:56 Changed 8 months ago by
- Commit changed from 87cba2d211c112741350d60a4d495b1de02f5c97 to c34c014645f04987796df7134c158dd8a5b05486
comment:57 Changed 8 months ago by
- 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
- Commit changed from c34c014645f04987796df7134c158dd8a5b05486 to 339fd660b7d6dfefb9527bc524ea9c71aa57465e
Branch pushed to git repo; I updated commit sha1. New commits:
339fd66 | Fix typo
|
comment:59 follow-up: ↓ 60 Changed 8 months ago by
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
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
- Description modified (diff)
comment:62 Changed 5 months ago by
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
- Description modified (diff)
comment:64 Changed 5 months ago by
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: ↓ 66 Changed 5 months ago by
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
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
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
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: ↓ 71 Changed 5 months ago by
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: ↓ 72 Changed 5 months ago by
And actually I don't think startup_guard
should be a new separate module.
comment:71 in reply to: ↑ 69 ; follow-up: ↓ 74 Changed 5 months ago by
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
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: ↓ 75 Changed 5 months ago by
Where it was - lazy_import.
comment:74 in reply to: ↑ 71 ; follow-up: ↓ 76 Changed 5 months ago by
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: ↓ 78 ↓ 79 Changed 5 months ago by
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: ↓ 77 Changed 5 months ago by
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
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
- Status changed from needs_review to needs_work
comment:79 in reply to: ↑ 75 ; follow-up: ↓ 80 Changed 5 months ago by
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
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
- Status changed from needs_work to needs_review
comment:82 Changed 4 months ago by
- 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
- Milestone changed from sage-9.6 to sage-9.7
comment:84 Changed 4 months ago by
- Commit changed from 339fd660b7d6dfefb9527bc524ea9c71aa57465e to 998e9e62af2a25792d0450af0a7f23cc7f4225a1
Branch pushed to git repo; I updated commit sha1. New commits:
998e9e6 | Merge branch 'develop' into public/build/startupWarning
|
comment:85 Changed 4 months ago by
- Commit changed from 998e9e62af2a25792d0450af0a7f23cc7f4225a1 to 567465572d55907dae5f790385e3663c64d3c838
Branch pushed to git repo; I updated commit sha1. New commits:
5674655 | Narrow down scope of lock
|
comment:86 Changed 4 months ago by
I've narrowed down the scope of the lock/guard. What do you think?
comment:87 follow-up: ↓ 90 Changed 4 months ago by
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
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: ↓ 91 Changed 4 months ago by
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
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
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
- Commit changed from 567465572d55907dae5f790385e3663c64d3c838 to 264aa53e8b47dd5ebb3b53d26f8f47c8ff2d5beb
comment:93 Changed 3 months ago by
- Status changed from needs_work to needs_review
comment:94 follow-up: ↓ 95 Changed 3 months ago by
- 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: ↓ 96 Changed 3 months ago by
- 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.
comment:96 in reply to: ↑ 95 Changed 3 months ago by
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: ↓ 98 Changed 3 months ago by
In other words, still comment:74
comment:98 in reply to: ↑ 97 ; follow-up: ↓ 99 Changed 3 months ago by
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
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
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: ↓ 102 Changed 3 months ago by
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: ↓ 103 Changed 3 months ago by
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
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
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: ↓ 106 ↓ 107 ↓ 108 Changed 3 months ago by
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
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: ↓ 109 Changed 3 months ago by
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: ↓ 110 Changed 3 months ago by
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: ↓ 113 Changed 2 months ago by
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
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:111 Changed 2 months ago by
In comment:95 you called it a "sequential lock". It isn't.
comment:112 Changed 2 months ago by
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
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: ↓ 115 Changed 2 months ago by
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
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: ↓ 117 Changed 2 months ago by
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: ↓ 119 Changed 2 months ago by
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.
comment:118 follow-up: ↓ 120 Changed 2 months ago by
comment:119 in reply to: ↑ 117 ; follow-up: ↓ 123 Changed 2 months ago by
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 2However, this sequence will be executed when a user reloads the
sage.all
module usingimportlib.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: ↓ 121 ↓ 122 Changed 2 months ago by
Replying to mkoeppe:
On the other hand, this structure:
with ...: with ...: Block 1 with ...: Block 2 Block 3would 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: ↓ 124 Changed 2 months ago by
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
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: ↓ 125 Changed 2 months ago by
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 usingimportlib.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: ↓ 126 Changed 2 months ago by
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: ↓ 127 Changed 2 months ago by
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 usingimportlib.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
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
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
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.
Perhaps the start_startup....finish_startup could be a context manager?