Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Cleanup #1422

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from
Open

Cleanup #1422

wants to merge 19 commits into from

Conversation

MasterSkepticista
Copy link
Member

@MasterSkepticista MasterSkepticista commented Mar 5, 2025

Summary of changes:

  • Dead code elimination (remnant of interactive API like TASK_REGISTRY)
  • delta_updates -> use_delta_updates, to make it consistent with aggregator that went through a similar change.
  • All loggers module level (!). No more passing around self.logger.
  • Headers are created in a stateless manner for grpc client and server. See 2372d1e

Prerequisite for a larger refactor. Did not want to burden refactor PRs with cleanups like these.

Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
@rahulga1 rahulga1 self-requested a review March 5, 2025 04:57
@rahulga1
Copy link
Collaborator

rahulga1 commented Mar 5, 2025

@MasterSkepticista can you please keep the PR in WIP/Draft state when it is being updated, that will avoid confusion for ppl to review it pre-maturely.

@MasterSkepticista MasterSkepticista changed the title Cleanup [WIP] Cleanup Mar 5, 2025
@MasterSkepticista
Copy link
Member Author

@rahulga1 Sure. Marked as WIP. I have kept it in this state for CI to trigger, hope this is ok.

Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
@MasterSkepticista MasterSkepticista force-pushed the karansh1/dce branch 2 times, most recently from 203e4d3 to 2372d1e Compare March 5, 2025 05:30
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
@MasterSkepticista MasterSkepticista changed the title [WIP] Cleanup Cleanup Mar 5, 2025
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
@MasterSkepticista MasterSkepticista force-pushed the karansh1/dce branch 2 times, most recently from e2338a4 to 50ebdef Compare March 5, 2025 10:32
@MasterSkepticista
Copy link
Member Author

Ready for review.

P.S.: Thanks @noopurintel for digging this 50ebdef

Copy link
Contributor

@psfoley psfoley left a comment

Choose a reason for hiding this comment

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

Overall changes look good here, @MasterSkepticista! use_delta_updates parameter name will also need to be updated in OpenFL-Security once this is merged.

Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
Signed-off-by: Shah, Karan <kbshah1998@outlook.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants