Skip to content

alpenglow: add feature gate #6330

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AshwinSekar
Copy link

@AshwinSekar AshwinSekar commented May 27, 2025

Add new feature gate for alpenglow with a dummy key.

Once alpenglow stabilizes and is ready for activation, this will be rekeyed to a proper one.

@AshwinSekar AshwinSekar moved this to In Progress in Alpenglow May 27, 2025
Copy link

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Will y'all be adding more stuff behind this feature gate eventually ? If we change stuff behind the feature gate, we should hypothetically re-key the feature.

Can you elaborate a bit on the plan™️

@AshwinSekar
Copy link
Author

Will y'all be adding more stuff behind this feature gate eventually ? If we change stuff behind the feature gate, we should hypothetically re-key the feature.

Can you elaborate a bit on the plan™️

you're too speedy haha, was going to post on slack in a bit. the plan was to slowly migrate the "non-alpenglow" related stuff from the private repo to agave, and then add the new alpenglow logic once it stabilizes.

This involves the feature gate, and turning off poh / tower bft consensus related things behind the gate. wanted to get this out first so that non-consensus folks can have a chance to review and our foot is in the door for merge conflicts.

Your point about rekeying makes sense, I can put a dummy key in for now and use the real one once we are closer to release

@AshwinSekar AshwinSekar force-pushed the alpenglow branch 2 times, most recently from 91d335f to 57cc2da Compare May 27, 2025 21:20
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82.8%. Comparing base (4ba219c) to head (88a10be).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6330   +/-   ##
=======================================
  Coverage    82.8%    82.8%           
=======================================
  Files         846      846           
  Lines      378217   378225    +8     
=======================================
+ Hits       313234   313312   +78     
+ Misses      64983    64913   -70     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@AshwinSekar AshwinSekar requested review from carllin and ksn6 May 28, 2025 01:03
@AshwinSekar AshwinSekar marked this pull request as draft May 28, 2025 02:21
// Activate all features at genesis in development mode
for feature_id in FeatureSet::default().inactive() {
activate_feature(genesis_config, *feature_id);
if IS_ALPENGLOW || *feature_id != agave_feature_set::alpenglow::id() {
Copy link

Choose a reason for hiding this comment

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

To confirm, the idea here is keep AG feature disabled by default but to also have a way to enable the gate for testing ?

Copy link
Author

Choose a reason for hiding this comment

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

yeah exactly. it will take several dozen PRs before alpenglow is fully functional on master during which all our existing local cluster tests will be broken.

once functional we can remove this exception to enable the existing tests before we consider rekeying and releasing.

Copy link

Choose a reason for hiding this comment

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

Got it, that seems reasonable if we're going to incrementally move AG stuff over

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants