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.

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

Migrate defaults to a separate file #1356

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ishaileshpant
Copy link
Collaborator

@ishaileshpant ishaileshpant commented Feb 9, 2025

This PR refactors the aggregator and collaborator modules by replacing hard-coded “magic” numbers and strings with centralized constant definitions. A new file, constants.py, has been introduced in the openfl/component directory to store these default values. This change improves maintainability and consistency across the codebase.

Motivation

  • Avoiding Magic Numbers/Strings:

Hard-coded values (e.g., 256, "tensor.db", "RESET", etc.) were scattered in the code. Extracting them into a constants file helps prevent errors and makes future updates easier.

  • Enhanced Readability & Maintainability:

Using descriptive constant names (like ROUNDS_TO_TRAIN and CERT_COMMON_NAME) clarifies the purpose of each value and allows for a single point of modification if defaults need to change.

@ishaileshpant ishaileshpant changed the title Pull out magic numbers to constants Pull out magic numbers/strings to constants Feb 9, 2025
@ishaileshpant ishaileshpant marked this pull request as ready for review February 10, 2025 10:38
@ishaileshpant ishaileshpant changed the title Pull out magic numbers/strings to constants Pull out magic numbers/strings in aggregator/collaborator Feb 10, 2025
Copy link
Member

@MasterSkepticista MasterSkepticista left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up @ishaileshpant.

Two suggestions:

  • It is recommended to keep constants at a module-level.
  • Can remove DEFAULT_ string for all constants. Upper case obviates the need to be explicit.

@ishaileshpant
Copy link
Collaborator Author

Thanks for picking this up @ishaileshpant.

Two suggestions:

  • It is recommended to keep constants at a module-level.
  • Can remove DEFAULT_ string for all constants. Upper case obviates the need to be explicit.

The only case that it won't address is some constants span across modules and hence i move to global package level else duplication of constants will be there

agree on the "default" prefix, in fact we can even name the file as defaults, that would further simplify the names and purpose becomes explicit

…d aggregator

- Rebased 10.Feb.2
Signed-off-by: Shailesh Pant <[email protected]>
@MasterSkepticista MasterSkepticista changed the title Pull out magic numbers/strings in aggregator/collaborator Migrate defaults to a separate file Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants