8000 js plugin lifecycle methods are not awaited · Issue #1474 · pyscript/pyscript · GitHub
[go: up one dir, main page]
Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Closed
3 tasks done
bugzpodder opened this issue May 18, 2023 · 19 comments
Closed
3 tasks done

js plugin lifecycle methods are not awaited #1474

bugzpodder opened this issue May 18, 2023 · 19 comments
Assignees
@WebReflection
Labels
needs-triage Issue needs triage type: bug Something isn't working

Comments

@bugzpodder
Copy link
Contributor
bugzpodder commented May 18, 2023 •
edited
Loading

Checklist

  • I added a descriptive title
  • I searched for other issues and couldn't find a solution or duplication
  • I already searched in Google and didn't find any good information or help

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
@bugzpodder bugzpodder added needs-triage Issue needs triage type: bug Something isn't working labels May 18, 2023
@bugzpodder bugzpodder mentioned this issue May 18, 2023
Closed
3 tasks
@WebReflection
Copy link
Contributor

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!

@WebReflection
Copy link
Contributor

on a second look I think all other methods should behave the same but I wonder:

  • what was the rationale for having only python plugins await
  • do we need to await one after the other or could Promise.all instead?
  • should we Promise.all all other plugins too or do we need to await them?

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

@FabioRosado
Copy link
Contributor

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 😅

@WebReflection
Copy link
Contributor

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?

@FabioRosado
Copy link
Contributor

@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 😄

@JeffersGlass
Copy link
Contributor

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 Promise.all() for all the plugin methods of a given hook at the moment... like @FabioRosado says, having plugins be able to "depend" on each other (in many and loose meanings of that word) is a long term goal, but it's not currently a part of the behavior we're depending on. I could see an argument for treating the internal splashscreen plugin as special potentially, so that it always appears as soon as possible in beforeLaunch and closes as soon (or as late??) as possible in afterStartup, but that's just an impulse.

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.

@WebReflection WebReflection self-assigned this May 22, 2023
@WebReflection
Copy link
Contributor

Thanks @JeffersGlass I've landed a PR to hopefully manage this #1481

@hoodmane
Copy link
Contributor

with Synclink any Python executions must be awaited

This isn't quite true, you can do Promise.resolve(task) or task.scheduleAsync() and that will schedule the task but not block for it. But it's probably better to actually block for the tasks.

We're still very inconsistent about our handling of asyncio...

@WebReflection WebReflection moved this to Working in PyScript OSS May 22, 2023
@fpliger
Copy link
Contributor
fpliger commented May 22, 2023

@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 packages dependencies).

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

@antocuni
Copy link
Contributor

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 for using Promise.all()
  • +1 for explicitly awaiting all the plugin hooks, one by one

8000
@bugzpodder
Copy link
Contributor Author
bugzpodder commented May 23, 2023 •
edited
Loading

So, unpopular opinion of the day:

  • -1 for using Promise.all()
  • +1 for explicitly awaiting all the plugin hooks, one by one

+1 from me as well

@WebReflection
Copy link
Contributor
WebReflection commented May 24, 2023 •
edited
Loading

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:

  • because we don't have any resolution for dependencies in place, nothing literally changes at the order level for JS plugins because these are in fact executed in order just like they did before
  • if somebody would like to couple a JS plugin with a Python one (see OP and issue description) the order is predictable and granted (JS first)
  • because plugins dependencies can have pinned versions, there's no point in guaranteeing the order
  • because we don't handle plugins dependencies in any way, there's no logic that would guarantee anything at any time

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 await one after the other but I am usually not super happy to add extra ticks all over the place for logic and use cases that don't exist in the real world ... here I just want to unlock the issue and move forward, and I think the MR did it, for what we know and we also test, in the most optimal way, but then again it takes nothing to await all the things and delay the bootstrap but in the future we'll need to change that code/logic regardless because no dependencies management is in place anyway.

@WebReflection
Copy link
Contributor
WebReflection commented May 24, 2023 •
edited
Loading

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 await per each JS plugin instead, thank you!

@fpliger
Copy link
Contributor
fpliger commented May 24, 2023

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:

So, unpopular opinion of the day:

-1 for using Promise.all()
+1 for explicitly awaiting all the plugin hooks, one by one

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.

  • because plugins dependencies can have pinned versions, there's no point in guaranteeing the order
  • because we don't handle plugins dependencies in any way, there's no logic that would guarantee anything at any time

I assume you are talking about package dependencies and not inter-plugins dependencies...

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.

👆🏼

@WebReflection
Copy link
Contributor

I am a bit confused now ... should I change the currently working MR to just await all the things?

I assume you are talking about package dependencies and not inter-plugins dependencies...

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.

@WebReflection
Copy link
Contributor

@fpliger to clarify my confusion, you wrote:

Wow. I agree with @antocuni

which means the MR you approved is not good to go as it uses Promise.all() and not a single await for every single thing, promise or not ... I am OK either way except I need to change the whole MR if await everywhere is preferred.

@fpliger
Copy link
Contributor
fpliger commented May 24, 2023

@WebReflection, sorry for the confusion. My position is:

  • I agree with @antocuni in that is a very predictable order of execution and a good starting point (as it's the simpler one) and would be my preference as a starting point
  • I agree with you that your PR is good enough to solve the problem here and move ahead. We can merge it. As we work on the rest of the plugins features for the release, we'll probably have a better view into the implications of one approach and the other. (aka, it's not a written in stone change)

@WebReflection
Copy link
Contributor
WebReflection commented May 24, 2023 via email

@fpliger
Copy link
Contributor
fpliger commented May 24, 2023 •
edited
Loading

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

Agreed. +1, let's merge it

@WebReflection WebReflection moved this from Working to Closed in PyScript OSS May 25, 2023
Sign up for free to join this conversation on G 5C55 itHub. Already have an account? Sign in to comment
Assignees

@WebReflection WebReflection

Labels
needs-triage Issue needs triage type: bug Something isn't working
Projects
PyScript OSS
Status: Closed
Milestone
No milestone
Development

No branches or pull requests
7 participants
@WebReflection @antocuni @fpliger @JeffersGlass @FabioRosado @hoodmane @bugzpodder
0