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

Query for fresh workspace envs when autoselecting interpreters in a new workspace #17264

Merged
merged 6 commits into from
Sep 8, 2021

Conversation

karrtikr
Copy link

@karrtikr karrtikr commented Sep 3, 2021

) {
super([...nonWorkspace, workspace]);
}

public iterEnvs(query?: PythonLocatorQuery): IPythonEnvsIterator<I> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds support for only querying for workspace envs.

/**
* Force an initial background refresh of the environments.
*
* Note API is ready to be queried only after a refresh has been triggered, and extension activation is blocked on API. So,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Various stuff like autoselection, jupyter integration requires the API, so it needs to be ready.

*/
includeNonRooted?: boolean;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the behavior to include non workspace envs by default.

@karrtikr karrtikr changed the title Only ignore workspace interpreter cache when autoselecting Query for fresh workspace envs when autoselecting interpreters in a new workspace Sep 8, 2021
@karrtikr karrtikr marked this pull request as ready for review September 8, 2021 01:48
* * If discovery was never triggered, we need to block extension activation on the refresh trigger.
* * If discovery was already triggered, there's no need to block extension activation on discovery.
*/
const wasTriggered = getGlobalStorage<boolean>(ext.context, 'PYTHON_WAS_DISCOVERY_TRIGGERED', false);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, this checks if discovery was ever triggered, not just for this session?

Does this mean that the extension activation will block on discovery for all users once when this PR lands, even though they might have already discovered interpreters before?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, this checks if discovery was ever triggered, not just for this session?

Yep, that is correct.

Does this mean that the extension activation will block on discovery for all users once when this PR lands, even though they might have already discovered interpreters before?

No, quite the opposite actually. If they have already discovered interpreters, extension activation will not block on discovery.

We only intend to block on triggering discovery the first time when extension loads.

Copy link

@kimadeline kimadeline Sep 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PYTHON_WAS_DISCOVERY_TRIGGERED key does not exist at the moment, so as soon as this PR gets merged the code will go through the condition on L.74-77 and wait on the api.triggerRefresh() part for everyone the first time they launch the extension, even users who have used the extension before and may have triggered discovery before.

Do we have any existing flags to see if any interpreters have ever been discovered?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you mean. We do have existing flags we can check but the one-time cost should be fine here, as this was always the behavior.

@karrtikr karrtikr merged commit be8cf55 into microsoft:main Sep 8, 2021
@karrtikr karrtikr deleted the optimizerefresh branch September 8, 2021 22:31
@karrtikr karrtikr added this to the September 2021 milestone Sep 9, 2021
@karrtikr karrtikr added the verified Verification succeeded label Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
verified Verification succeeded
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants