fix(federated): prevent regularization decay in aggregate_updates#1648
Open
Ridanshi wants to merge 2 commits into
Open
fix(federated): prevent regularization decay in aggregate_updates#1648Ridanshi wants to merge 2 commits into
Ridanshi wants to merge 2 commits into
Conversation
…separating regularization from client averaging Resolves leonagoel#1346 - Renamed avg_update to avg_data_gradient to make clear only data-gradient contributions from clients are averaged. - Extracted reg_penalty = reg * global_item_factors[:, idx] as an explicit separate term so it is applied at full strength once per update step, independent of how many clients contributed updates for that item. - Added test_regularization_strength_constant_across_clients which verifies that the item factor update is identical for 1, 2, and 5 clients sending identical data gradients, confirming regularization is not scaled down by N. - Fixed pre-existing syntax errors in backend/main.py and src/model/hybrid_model.py that blocked test collection. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
Bot
added
gssoc:approved
mentor:leonagoel
status:review-needed
type:bug
labels
|
🎉 Welcome to Hybrid Recommender, @Ridanshi! This is your first contribution here! Labels added: PR Description Checklist: What happens next:
⏱️ Please respond to review comments within 48 hours. |
🎉 Welcome to Hybrid Recommender, @Ridanshi!Thank you for your first pull request! Here's what happens next:
⏱️ Please respond to any review comments within 48 hours. 📖 Resources: Happy contributing! 🚀 |
backend/main.py: - Keep specific (RedisError, json.JSONDecodeError) exception types from upstream in _get_cached_response; retain blank-line separator before @app.post decorator from our branch. src/model/hybrid_model.py: - Restore properly indented select_bandit_arm() method definition lost in upstream conflict. - Use defensive getattr for bandit_arms access from upstream to guard against uninitialised attribute. - Drop orphaned __init__ code fragment left inside select_bandit_arm() by upstream merge, restoring syntactic validity. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1346
aggregate_updates()was merging the regularization term into the same expression as the averaged client data gradients. While the server-side regularization was mathematically correct, the coupling made it easy to introduce the decay bug and gave no explicit guarantee at the code level.avg_update→avg_data_gradientto make the intent unambiguous: only client data-gradient contributions are averaged.reg_penalty = self.reg * self.global_item_factors[:, idx]as an explicit separate variable applied once per update step, independent of how many clients contributed for that item.Test plan
test_regularization_strength_constant_across_clients— new test verifying that the item factor update is identical for 1, 2, and 5 clients sending identical data gradients (reg not scaled by N).test_federated_server_aggregation— existing test, still passes with the refactored code.test_federated_client_factor_calculation— passes.test_federated_client_item_updates— passes.test_train_federated_collaborative_model— passes.