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.

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

Call the correct API to determine if a user is in treatment or control group #20183

Closed
luabud opened this issue Nov 8, 2022 · 6 comments · Fixed by #20690
Closed

Call the correct API to determine if a user is in treatment or control group #20183

luabud opened this issue Nov 8, 2022 · 6 comments · Fixed by #20690
Assignees
@karrtikr
Labels
area-internal Label for non-user facing issues debt Covers everything internal: CI, testing, refactoring of the codebase, etc. important Issue identified as high-priority needs spike Label for issues that need investigation before they can be worked on. verified Verification succeeded
Milestone
February 2023

Comments

@luabud
Copy link
Member

luabud commented Nov 8, 2022

We seem to be using isInExperimentSync but that returns whether a user is in treatment or control, but we should probably be using getTreatmentVariable? This issue is so we can figure this out.
@luabud luabud added debt Covers everything internal: CI, testing, refactoring of the codebase, etc. area-internal Label for non-user facing issues needs spike Label for issues that need investigation before they can be worked on. labels Nov 8, 2022
@karrtikr karrtikr added the important Issue identified as high-priority label Nov 8, 2022
@kieferrm kieferrm mentioned this issue Jan 2, 2023
Closed
34 tasks
@luabud
Copy link
Member Author

luabud commented Jan 27, 2023

It really should be getExperimentValue: https://github.com/microsoft/tas-client/tree/main/vscode-tas-client#use

@luabud luabud closed this as completed Jan 27, 2023
@karrtikr
Copy link

Great, we don't have an issue tracking implementing this though, so can we reopen this?

@luabud luabud removed the needs spike Label for issues that need investigation before they can be worked on. label Feb 3, 2023
@luabud luabud reopened this Feb 3, 2023
@github-actions github-actions bot added the triage-needed Needs assignment to the proper sub-team label Feb 3, 2023
@luabud luabud changed the title Spike - what is the right API to call for retrieving experiments Use getTreatmentVariable instead of isInExperiment for running experiments Feb 3, 2023
@luabud
Copy link
Member Author

luabud commented Feb 3, 2023

Oops, good point :) thank you!

@kieferrm kieferrm mentioned this issue Feb 4, 2023
Closed
41 tasks
@karrtikr
Copy link

karrtikr commented Feb 8, 2023

@luabud Thing is inExperiment already uses getTreatmentVariable 🤷‍♂️:

vscode-python/src/client/common/experiments/service.ts

Line 143 in e92c124

const treatmentVariable = this.experimentationService.getTreatmentVariable(EXP_CONFIG_ID, experiment);

So back to square one, not sure what is to be used here.

@karrtikr karrtikr changed the title Use getTreatmentVariable instead of isInExperiment for running experiments Spike - what is the right API to call for retrieving experiments Feb 8, 2023
@karrtikr karrtikr added needs spike Label for issues that need investigation before they can be worked on. and removed triage-needed Needs assignment to the proper sub-team labels Feb 8, 2023
@karrtikr karrtikr removed their assignment Feb 8, 2023
@luabud
Copy link
Member Author

luabud commented Feb 10, 2023

@karrtikr but it returns treatmentVariable !== undefined;, which is true whether user is in treatment or control, no?

@karrtikr karrtikr self-assigned this Feb 13, 2023
@karrtikr karrtikr added this to the February 2023 milestone Feb 13, 2023
@karrtikr
Copy link

karrtikr commented Feb 13, 2023 •
edited
Loading

I think you were the one who observed this right? So as I understand the problem here is that how do we distinguish between control groups and treatment groups.

Digging a little bit:

call getTreatmentVariable(configId: string, name: string) to get the value of a treatment variable. configId is always vscode and name is the name of the treatment variable that you defined in control tower when you set up your experiment.

The doc says getTreatmentVariable returns the value of the treatment variable. And when we create an exp on control tower:

image

We generally set the value of treatment as true for experiment group, and false for control group. Given we always follow this convention, I think we should be checking if treatmentVariable === true instead. Does it make sense?

@karrtikr karrtikr mentioned this issue Feb 13, 2023
Merged
karrtikr pushed a commit that referenced this issue Feb 13, 2023
Call the correct API to determine if a user is in treatment or contro…
2202fbe 
…l group (#20690)

Closes #20183
@karrtikr karrtikr changed the title Spike - what is the right API to call for retrieving experiments Call the correct API to determine if a user is in treatment or control group Feb 13, 2023
@kieferrm kieferrm mentioned this issue Feb 21, 2023
Closed
20 tasks
@karthiknadig karthiknadig added the verified Verification succeeded label Feb 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2023
wesm pushed a commit to posit-dev/positron that referenced this issue Mar 28, 2024
@debonte @karthiknadig
@mitchell-as @petetronic @eleanorjboyd
Merge latest upstream python extension
34c70bb 
--------------------
Commit message for microsoft/vscode-python@fc72be9:

Show `Python: Report issue` command in palette regardless of whether a Python file is opened (microsoft/vscode-python#20726)

Closes microsoft/vscode-python#20723
--------------------
Commit message for microsoft/vscode-python@c18e8c9:

Detect ActiveState Python runtimes (microsoft/vscode-python#20534)

Closes microsoft/vscode-python#20532
--------------------
Commit message for microsoft/vscode-python@2152cd9:

Don't set `formatOnType` for auto-indent experiment if it's already set (microsoft/vscode-python#20710)


--------------------
Commit message for microsoft/vscode-python@995b0bc:

Add support for 'back' to all create env UI. (microsoft/vscode-python#20693)

Closes microsoft/vscode-python#20274


### Usage

This change allows callers of the Create Environment command to handle
`Back` and `Cancel`:
``` typescript
let result: CreateEnvironmentResult | undefined;
try {
    const result = await commands.executeCommand("python.createEnvironment", {showBackButton: true});
} catch(e) {
   // error while creating environment
}

if (result?.action === 'Back') {
    // user clicked Back
}

if (result?.action === 'Cancel') {
    // user pressed escape or Cancel
}
```
I decided to go with `result?.action` because we don't have a npm
package for python extension API so catching particular exception might
be error prone with `ex instanceof <error>`. We will provide a proper
interface via `api.environments` for create environment, and
contribution to create environment. Until that point this command will
provide the stop gap.

### Notes

1. I did not use the multi-step input that is used in the rest of the
extension because, the existing implementation does not have context.
Consider the following scenario: venv -> workspace select -> python
select -> packages. Assume that there is only one workspace, and we
don't show the workspace selection UI, that decision is done inside the
workspace step. So, if there is only 1 workspace it is a short circuit
to next step. User is on python selection and clicks `back`, workspace
selection short circuits to next step which is python selection. So,
from user perspective, back does not work. This can be fixed by sending
context that the reason control moved to previous step was because user
clicked on back.
2. This makes a change to old multi step API to rethrow the exception,
if user hits `back` and the current step has no steps to go back to.
--------------------
Commit message for microsoft/vscode-python@f3ecbf5:

Fix-conda-version-parsing (microsoft/vscode-python#20674)


--------------------
Commit message for microsoft/vscode-python@a6a6f50:

Inactive pytest run command (microsoft/vscode-python#20653)

Here the new flow is created but kept inactive for the pytest execution
--------------------
Commit message for microsoft/vscode-python@2202fbe:

Call the correct API to determine if a user is in treatment or control group (microsoft/vscode-python#20690)

Closes microsoft/vscode-python#20183
--------------------
Commit message for microsoft/vscode-python@b0ab10d:

Only use activated environment from terminal if VSCode was launched via CLI (microsoft/vscode-python#20667)

Closes microsoft/vscode-python#20644
--------------------
Commit message for microsoft/vscode-python@02a92fc:

Ensure interpreter path isn't truncated for workspace-relative paths when storing value (microsoft/vscode-python#20661)

For microsoft/vscode-python#20660

I'm not quite sure why this was done. It doesn't make sense to do this
only for display.
--------------------
Commit message for microsoft/vscode-python@377067f:

Use correct API to get interpreter path for language servers (microsoft/vscode-python#20656)

For microsoft/vscode-python#20644 closes
microsoft/vscode-python#20657
--------------------
Commit message for microsoft/vscode-python@cd6ca9d:

Remove `isort` extension dependency (microsoft/vscode-python#20577)

Closes microsoft/vscode-python#20586

Lead-authored-by: Kartik Raj <[email protected]>
Co-authored-by: Erik De Bonte <[email protected]>
Co-authored-by: Karthik Nadig <[email protected]>
Co-authored-by: mitchell <[email protected]>
Co-authored-by: Pete Farland <[email protected]>
Co-authored-by: Eleanor Boyd <[email protected]>
wesm pushed a commit to posit-dev/positron that referenced this issue Mar 28, 2024
@debonte @karthiknadig
@mitchell-as @petetronic @eleanorjboyd @wesm
Merge latest upstream python extension
8292d65 
--------------------
Commit message for microsoft/vscode-python@fc72be9:

Show `Python: Report issue` command in palette regardless of whether a Python file is opened (microsoft/vscode-python#20726)

Closes microsoft/vscode-python#20723
--------------------
Commit message for microsoft/vscode-python@c18e8c9:

Detect ActiveState Python runtimes (microsoft/vscode-python#20534)

Closes microsoft/vscode-python#20532
--------------------
Commit message for microsoft/vscode-python@2152cd9:

Don't set `formatOnType` for auto-indent experiment if it's already set (microsoft/vscode-python#20710)


--------------------
Commit message for microsoft/vscode-python@995b0bc:

Add support for 'back' to all create env UI. (microsoft/vscode-python#20693)

Closes microsoft/vscode-python#20274


### Usage

This change allows callers of the Create Environment command to handle
`Back` and `Cancel`:
``` typescript
let result: CreateEnvironmentResult | undefined;
try {
    const result = await commands.executeCommand("python.createEnvironment", {showBackButton: true});
} catch(e) {
   // error while creating environment
}

if (result?.action === 'Back') {
    // user clicked Back
}

if (result?.action === 'Cancel') {
    // user pressed escape or Cancel
}
```
I decided to go with `result?.action` because we don't have a npm
package for python extension API so catching particular exception might
be error prone with `ex instanceof <error>`. We will provide a proper
interface via `api.environments` for create environment, and
contribution to create environment. Until that point this command will
provide the stop gap.

### Notes

1. I did not use the multi-step input that is used in the rest of the
extension because, the existing implementation does not have context.
Consider the following scenario: venv -> workspace select -> python
select -> packages. Assume that there is only one workspace, and we
don't show the workspace selection UI, that decision is done inside the
workspace step. So, if there is only 1 workspace it is a short circuit
to next step. User is on python selection and clicks `back`, workspace
selection short circuits to next step which is python selection. So,
from user perspective, back does not work. This can be fixed by sending
context that the reason control moved to previous step was because user
clicked on back.
2. This makes a change to old multi step API to rethrow the exception,
if user hits `back` and the current step has no steps to go back to.
--------------------
Commit message for microsoft/vscode-python@f3ecbf5:

Fix-conda-version-parsing (microsoft/vscode-python#20674)


--------------------
Commit message for microsoft/vscode-python@a6a6f50:

Inactive pytest run command (microsoft/vscode-python#20653)

Here the new flow is created but kept inactive for the pytest execution
--------------------
Commit message for microsoft/vscode-python@2202fbe:

Call the correct API to determine if a user is in treatment or control group (microsoft/vscode-python#20690)

Closes microsoft/vscode-python#20183
--------------------
Commit message for microsoft/vscode-python@b0ab10d:

Only use activated environment from terminal if VSCode was launched via CLI (microsoft/vscode-python#20667)

Closes microsoft/vscode-python#20644
--------------------
Commit message for microsoft/vscode-python@02a92fc:

Ensure interpreter path isn't truncated for workspace-relative paths when storing value (microsoft/vscode-python#20661)

For microsoft/vscode-python#20660

I'm not quite sure why this was done. It doesn't make sense to do this
only for display.
--------------------
Commit message for microsoft/vscode-python@377067f:

Use correct API to get interpreter path for language servers (microsoft/vscode-python#20656)

For microsoft/vscode-python#20644 closes
microsoft/vscode-python#20657
--------------------
Commit message for microsoft/vscode-python@cd6ca9d:

Remove `isort` extension dependency (microsoft/vscode-python#20577)

Closes microsoft/vscode-python#20586

Lead-authored-by: Kartik Raj <[email protected]>
Co-authored-by: Erik De Bonte <[email protected]>
Co-authored-by: Karthik Nadig <[email protected]>
Co-authored-by: mitchell <[email protected]>
Co-authored-by: Pete Farland <[email protected]>
Co-authored-by: Eleanor Boyd <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@karrtikr karrtikr

Labels
area-internal Label for non-user facing issues debt Covers everything internal: CI, testing, refactoring of the codebase, etc. important Issue identified as high-priority needs spike Label for issues that need investigation before they can be worked on. verified Verification succeeded
Projects
None yet
Milestone
February 2023
Development

Successfully merging a pull request may close this issue.

Call the correct API to determine if a user is in treatment or control group karrtikr/vscode-python
3 participants
@karthiknadig @karrtikr @luabud