feat: remove genai from experimental feature list and enable via /master feature switches [GAS-1016]
#9435
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
hkang1
force-pushed
the
enable-gas-permanently
branch
from
d6abaa5
to
36ac6c3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9435 +/- ##
==========================================
- Coverage 51.06% 43.78% -7.29%
==========================================
Files 747 423 -324
Lines 111167 71219 -39948
Branches 2778 2781 +3
==========================================
- Hits 56772 31182 -25590
+ Misses 54220 39862 -14358
Partials 175 175
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
should this also be added to the mobile |
hkang1
force-pushed
the
enable-gas-permanently
branch
2 times, most recently
from
cabc4b7
to
0644c04
Compare
| @@ -37,6 +38,7 @@ export const FEATURES: Record<ValidFeature, FeatureDescription> = { | |||
| defaultValue: false, | |||
There was a problem hiding this comment.
also want defaultValue to be true, right?
There was a problem hiding this comment.
Actually we don't want this always on, because we still want featureSwitches from /api/v1/master to be able to return and control whether genai is supported or not. If the deployment does not support genai, then featureSwitches will not have genai in the list and the UI will not show the corresponding link either. Having it on makes it always appear no matter what for me.
hkang1
force-pushed
the
enable-gas-permanently
branch
from
0644c04
to
3db4de1
Compare
Good call with the tab bar, added to the tabbar as well |
hkang1
changed the title
/master feature switches [GAS-1016]
| featureSwitches.includes(feature) && (isOn = true); | ||
| featureSwitches.includes(`-${feature}`) && (isOn = false); |
There was a problem hiding this comment.
re: previous comment about defaultValue
I thought the intent was to enable the feature by default, but still be able to disable it via config. According to this check here, disabling via config would be accomplished by including -genai in featureSwitches, not by omitting it (omitting defers to default)
but if the intent is actually to have it disabled by default and only enabled via config, then this does seem to accomplish that.
There was a problem hiding this comment.
ah yeah great point, since I wasn't aware of the -genai possibility. I confirmed with the team that genai will still be off by default on the config side and that it has to be explicitly turned on to enable it. So on the featureSwitches side it should only ever come through as genai (without the minus prefix).
| <Select searchable={false}> | ||
| <Option value={true}>On</Option> | ||
| <Option value={false}>Off</Option> | ||
| </Select> |
There was a problem hiding this comment.
nit: ET team has discussed wanting to replace these Selects with Toggles. definitely outside the scope of this PR but thought I'd bring it up since we're making changes to this area anyway.
There was a problem hiding this comment.
yeah, the toggle definitely makes more sense here for these binary options!
Nav Bar
Tab Bar
Removed
genaifrom Experimentals list of features.Ticket
GAS-1016
Description
Removes the user control of turning on/off genai from the Experimental list of features under user settings. The
noUserControlflag makes the feature purely controlled by the backendfeatureSwitches, where if it is included in there, then it is enabled.Test Plan
GenAIfeature does NOT show up on theExperimentalslist of features under user settings.f_genai=truedoes not turn GenAI on in an MLDE cluster that does not have genai enabled.genaireturned by the/masterendpoint (featureSwitches) then it appears in the sidebar.genaireturned by the/asterendpoint (featureSwitches) then it does NOT appear in the side bar.Checklist
docs/release-notes/.See Release Note for details.