Skip to content
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

Python API triggers add events even after refresh promise has resolved #20216

Closed
DonJayamanne opened this issue Nov 11, 2022 · 6 comments
Closed
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug info-needed Issue requires more information from poster triage-needed Needs assignment to the proper sub-team verified Verification succeeded
Milestone

Comments

@DonJayamanne
Copy link

Jupyter extension invokes the refreshEnvironments API method.
However even after this promise resolves, we get events being triggered for onDidChangeEnvironments.
Meaning the discovery has not been completed.

I.e. once refreshEnvironments, then this means all discovery has been completed, however events being triggered means this is still in progress.

I would not expect any more events to be triggered for onDidChangeEnvironments after the refreshEnvironments promise resolves. Similarly I would not expect any changes to known after this refresh promise.

Exceptions include users creating/removing python environments, however when it comes to refresh the expectation is that this promise should resolve after everything has been completed.

@DonJayamanne DonJayamanne added the bug Issue identified by VS Code Team member as probable bug label Nov 11, 2022
@github-actions github-actions bot added the triage-needed Needs assignment to the proper sub-team label Nov 11, 2022
@DonJayamanne
Copy link
Author

@karrtikr we need to know when all envs have been discovered and completed so we can have a progress indicator in the kernel quickpick (this is the quick pick sample I shared with you).
Based on todays API, promise completes early and we still get events for envs being added, and as a result of this we have a few issues & even bugs:

Bugs

  • The progress indicator ends early
  • We assume that all envs have been disvoered and then try to recommend kernels for the currently opened notebook, however since discovery is still in progress the jupyter extension doesn't get all envionments and we end up recommending the wrong python env.
    • If you were to reload vscode then we recommend the right kernel (i.e. at one point we recommend Python Env A, then later we recommend B, & tha'ts wrong)
  • Azure AML extension also needs to know when python env discovery is complete, as they rely on jupyter completing discovery of kernels, however if Python doesn't complete disocvery correctly then AML will not know when kernel discovery has been completed & AML too ends up with bugs (email sent to you separately)

@karrtikr
Copy link

karrtikr commented Nov 14, 2022

I tried a few times but I am not able to reproduce this issue using pre release Jupyter. Here's what I did:

  • Logged everytime we fired a change event with time
  • Logged when refreshEnvironment resolves with time
  • Simply opened a python file to load the extensions

All events seem be to fired before:
image

Please provide steps to repro.

@karrtikr karrtikr added the info-needed Issue requires more information from poster label Nov 14, 2022
@karrtikr
Copy link

I suspect the timings to be very close even if events are fired later. Please make sure you're not using any asynchronous callbacks while verifying.

@DonJayamanne
Copy link
Author

I suspect the timings to be very close even if events are fired later.

This doesn't make sense, are you saying that the current code does have the potential to trigger events after the refresh finishes?

Please make sure you're not using any asynchronous callbacks while verifying.

We do have a lot of async code, not sure how that would affect this. With async code the event loop should still handle the callbacks and the resolution of the promise in order.

@DonJayamanne
Copy link
Author

Damn, sorry, you are right, I can no longer repro this issue, must have been doing something wrong at my end.
Apologies for creating this issue.

@karrtikr
Copy link

We do have a lot of async code, not sure how that would affect this. With async code the event loop should still handle the callbacks and the resolution of the promise in order.

Each loop starts in order but can yield control in between if callback itself is async, so finish order is not guaranteed. Anyways glad you were able to verify its okay.

@karrtikr karrtikr added this to the November 2022 milestone Nov 14, 2022
@karrtikr karrtikr added the verified Verification succeeded label Nov 15, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug info-needed Issue requires more information from poster triage-needed Needs assignment to the proper sub-team verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

2 participants