-
Notifications
You must be signed in to change notification settings - Fork 1.5k
js plugin lifecycle methods are not awaited #1474
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
This seems a good call but I wonder if you could provide the most basic failing example you can think of so that we can integrate that example in our tests and be sure we won't break expectations in the future, thank you! |
on a second look I think all other methods should behave the same but I wonder:
/cc @antocuni @FabioRosado @fpliger anyone with more background around this decision? I think I've refactored the code but I didn't ask these questions while doing so and the previous behavior remained the same, thanks for any sort of hint. |
I don't know if there is any reason behind that. When I worked on implementing the js plugins, I didn't want to touch the whole logic of how the plugins get installed so I piggybacked on the logic for the python ones with some minor changes. It's likely that this decision was a terrible one since the python plugins were still experimental when I added the support for installing js ones 😅 |
so what do we want to do here? I say if python plugins can / must be awaited, then JS plugins should follow but I also keep wondering if it's needed to await sequentially rather than in parallel ... thoughts? |
@fpliger and perhaps @JeffersGlass will probably be the right people to say but I think we probably don't need to await each sequentially and they can be installed in parallel, unless we have one plugin that depends on other but maybe we don't care about that use case for now? In this specific case, I think we should probably add logic in the plugin config to say before you install this confirm that x is installed already but that would mean a better plugin config - which we mentioned in the past 😄 |
Originally the Python plugins were no awaited - it looks like that got added as part of the first part of synclink integration (see plugin.ts), since with Synclink any Python executions must be awaited. I don't think those awaits have a further purpose beyond that, i.e. allowing users to write async plugin methods. I think we could do a For what it's worth, this discussion essentially summarizes the ideas we had for plugin file format coming out of Lisbon in February. It's mostly just adding features to individual plugins, and doesn't deal with plugin dependencies/resolution, which we didn't get too deep into. |
Thanks @JeffersGlass I've landed a PR to hopefully manage this #1481 |
This isn't quite true, you can do We're still very inconsistent about our handling of asyncio... |
@JeffersGlass is correct here and we didn't specifically talked about plugins being awaited or dealing with plugins dependency/resolution (although we did talk about plugins Right now I don't think we have enough use cases and have exercised Plugins enough to make a "perfect" call but I kind of feel that #1481 awaiting all the same plugins lifecycles at once is in the right direction. Once we've moved the aspects of #1228 forward we can add the notion of specificy plugin-plugin dependencies |
A bit more historical context: plugins were introduced by PR #917 : at that point in time, everything was sync and the order of execution was stable and predictable. Then, some (but not all) of the plugin hooks were made async by #1258, as Jeff pointed out: this is were the "non-await" behavior was introduced. Personally, I think it's a mistake: having a predictable execution order is an important property and we should try hard to preserve it as much as possible: if we don't, then it's just a matter of time before we have plugins interacting with each other in unpredictable ways, and end user apps which works only depending on the phase of the moon. For example, I can imagine situations in which a particular app+plugins combination works on "fast connections" but is flaky on slow connections (just because whoever writes the app never has a fast one). I'm pretty sure we don't want that. So, unpopular opinion of the day:
|
+1 from me as well |
I think speculative-driven-development is very close to premature optimizations and I personally prefer @JeffersGlass pragmatism in saying that "we don't have enough information/experience around plugins" and my MR does just this:
In few words, the current situation is not ideal on the JS side and it solves nothing on the Python side except that asynchronous plugins are awaited because the pyodide initialization with plugins requires that before being able to run. Once we introduce packages dependencies handling and we use plugins names, not just their callback, we can revisit what has been improved already in my PR but that being said, if there are people really unhappy to parallelize and speed up the bootstrap dance, I am OK to |
P.S. to clarify why I think current MR is "good enough", the day we'll handle dependencies we'll need to fetch (in parallel) all dependencies per each plugin, create a dependencies graph which must preserve the order and yet import in parallel all dependencies that have no dependencies listed. This means that once we decide how or when dependencies between plugins are relevant, there will be much more code to change and more things to get right with tests and so on. Until then, could we just move forward? All current tests are green and, as we know, we don't have plugins dependencies resolution in place but if anyone knows of a real-world use case where on either the JS or the python level that's necessary, please let me know so I can update the MR (or revert it) and just add |
tldr: agree with the many statements around "we don't know enough" and that this unlocks the immediate issue. So I'm +1 on merging More details:
Wow. I agree with @antocuni (hence tropical storm here last night 🤣 ). More specifically, in the context of "not knowing enough", I think this helps simplify the base from where we start learning.
I assume you are talking about package dependencies and not inter-plugins dependencies...
👆🏼 |
I am a bit confused now ... should I change the currently working MR to just await all the things?
a plugin that depends on other plugins means composition for more complex scenarios so I am talking about any dependency we want to tackle in the future, including other plugins when current plugin depends on it/those. |
@fpliger to clarify my confusion, you wrote:
which means the MR you approved is not good to go as it uses |
@WebReflection, sorry for the confusion. My position is:
|
FWIWI we don't have any predictable order right now on the JS side, because
async JS plugins just run and nobody knows when these are ready, which is
the whole point of the Original Post. My MR grants that JS plugins are
awaited before Python plugins are also awaited, and the order we grant for
Python is not because it was a conscious design, it's only because we
introduced later on some asynchronous plugin that never relied on, or
needed, ordered plugins injection/execution.
This is why I think my MR is a slight improvement over what we hat before,
but it doesn't take into account order ... like before (or at least, not by
need).
…On Wed, May 24, 2023 at 5:44 PM Jeff Glass ***@***.***> wrote:
@fpliger <https://github.com/fpliger> I guess I too am confused, as those
are two opposite ideas. Should we be having a predictable execution order
right now (which is what we *do* have right now, even if it is poorly
documented) or should we *not* have a predictable execution order right
now, and merge @WebReflection <https://github.com/WebReflection>'s MR
with Promise.all(...)?
—
Reply to this email directly, view it on GitHub
<#1474 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAU55P2URD7ATKQJFPZ723XHYUGBANCNFSM6AAAAAAYGLEDP4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Agreed. +1, let's merge it |
Uh oh!
There was an error while loading. Please reload this page.
Checklist
What happened?
unlike python plugins, js plugin lifecycle methods are not awaited
I was repurposing a plugin for automatically install imported packages and couldn't get the js plugin to work because of this because
interpreter.remote.installPackage
is async.What browsers are you seeing the problem on? (if applicable)
No response
Console info
No response
Additional Context
No response
The text was updated successfully, but these errors were encountered: