Skip to content

Do not add to env cache if already present#15108

Merged
karrtikr merged 4 commits into
microsoft:mainfrom
karrtikr:caching
Jan 8, 2021
Merged

Do not add to env cache if already present#15108
karrtikr merged 4 commits into
microsoft:mainfrom
karrtikr:caching

Conversation

@karrtikr
Copy link
Copy Markdown

@karrtikr karrtikr commented Jan 7, 2021

For: Dups envs showing up when creating env in workspace on Windows

Environment was being added to cache list midway, and then was again pushed to list. So duplicate entries for the same env were showing up.

Linting errors not related to the PR.

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • The wiki is updated with any design decisions/details.

@karrtikr karrtikr added no-changelog No news entry required skip tests Updates to tests unnecessary labels Jan 7, 2021
@karrtikr karrtikr removed the skip tests Updates to tests unnecessary label Jan 8, 2021
@kimadeline
Copy link
Copy Markdown

See #15109 for the linting fix

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #15108 (feb4eea) into main (4284057) will increase coverage by 0%.
The diff coverage is 75%.

@@          Coverage Diff           @@
##            main   #15108   +/-   ##
======================================
  Coverage     65%      65%           
======================================
  Files        557      558    +1     
  Lines      26220    26226    +6     
  Branches    3729     3725    -4     
======================================
+ Hits       17061    17074   +13     
+ Misses      8463     8460    -3     
+ Partials     696      692    -4     
Impacted Files Coverage Δ
...ronments/base/locators/composite/cachingLocator.ts 80% <75%> (-1%) ⬇️
src/client/tensorBoard/helpers.ts 76% <0%> (-6%) ⬇️
src/client/common/utils/decorators.ts 78% <0%> (-1%) ⬇️
src/client/linters/bandit.ts 46% <0%> (ø)
src/client/tensorBoard/tensorBoardPrompt.ts 97% <0%> (ø)
...t/common/variables/environmentVariablesProvider.ts 93% <0%> (+1%) ⬆️
src/client/tensorBoard/tensorBoardImportTracker.ts 93% <0%> (+4%) ⬆️
src/client/common/utils/cacheUtils.ts 89% <0%> (+6%) ⬆️

Comment thread src/client/pythonEnvironments/base/locators/composite/cachingLocator.ts Outdated
Comment on lines +58 to +72
/**
* If the cache has been activated, return environment info objects that match a env.
* If none of the environments in the cache match the env, return an empty array.
*/
private filterMatchingEnvsFromCache(env: string | PythonEnvInfo): PythonEnvInfo[] {
// If necessary we could be more aggressive about invalidating
// the cached value.
const query = getMinimalPartialInfo(env);
if (query === undefined) {
return [];
}
const candidates = this.cache.filterEnvs(query);
return candidates !== undefined && candidates.length > 0 ? candidates : [];
}

Copy link
Copy Markdown
Author

@karrtikr karrtikr Jan 8, 2021

Choose a reason for hiding this comment

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

@kimadeline Ideally this is the filterEnvs the cache should have exposed instead of the one it does.

@karrtikr karrtikr merged commit 3ce3547 into microsoft:main Jan 8, 2021
@karrtikr karrtikr deleted the caching branch January 8, 2021 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog No news entry required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants