Skip to content

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

Closed
Closed
Python API triggers add events even after refresh promise has resolved#20216
Assignees
karrtikr
Labels
bugIssue identified by VS Code Team member as probable buginfo-neededIssue requires more information from postertriage-neededNeeds assignment to the proper sub-teamverifiedVerification succeeded
Milestone
November 2022

Description

@DonJayamanne
Don Jayamanne (DonJayamanne)
opened on Nov 11, 2022

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.

Activity

  1. Don Jayamanne (DonJayamanne)
    added
    bugIssue identified by VS Code Team member as probable bug
    on Nov 11, 2022
  2. Don Jayamanne (DonJayamanne)
    assigned
    Kartik Raj (karrtikr)
    on Nov 11, 2022
  3. github-actions
    added
    triage-neededNeeds assignment to the proper sub-team
    on Nov 11, 2022

  4. DonJayamanne commented on Nov 13, 2022

    @DonJayamanne
    Don Jayamanne (DonJayamanne)
    on Nov 13, 2022
    Author

    Kartik Raj (@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)

  5. karrtikr commented on Nov 14, 2022

    @karrtikr
    Kartik Raj (karrtikr)
    on 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.

  6. Kartik Raj (karrtikr)
    added
    info-neededIssue requires more information from poster
    on Nov 14, 2022

  7. karrtikr commented on Nov 14, 2022

    @karrtikr
    Kartik Raj (karrtikr)
    on Nov 14, 2022

    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.

  8. DonJayamanne commented on Nov 14, 2022

    @DonJayamanne
    Don Jayamanne (DonJayamanne)
    on Nov 14, 2022
    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.

  9. DonJayamanne commented on Nov 14, 2022

    @DonJayamanne
    Don Jayamanne (DonJayamanne)
    on Nov 14, 2022
    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.

  10. Don Jayamanne (DonJayamanne)
    closed this as completedon Nov 14, 2022

  11. karrtikr commented on Nov 14, 2022

    @karrtikr
    Kartik Raj (karrtikr)
    on Nov 14, 2022

    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.

  12. Kartik Raj (karrtikr)
    added this to the November 2022 milestone on Nov 14, 2022
  13. Kartik Raj (karrtikr)
    added
    verifiedVerification succeeded
    on Nov 15, 2022
  14. github-actions
    locked as resolved and limited conversation to collaborators on Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugIssue identified by VS Code Team member as probable buginfo-neededIssue requires more information from postertriage-neededNeeds assignment to the proper sub-teamverifiedVerification succeeded

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions