Skip to content

Conversation

@YuanTingHsieh
Copy link
Collaborator

@YuanTingHsieh YuanTingHsieh commented Aug 7, 2025

Fix attribute access in FedJob component serialization for framework compatibility

Description

This PR fixes a critical bug in FedJob's component serialization logic where direct self.__dict__ access fails for classes that don't store attributes in the standard dictionary, such as PyTorch nn.Module and other framework classes that override __setattr__ behavior.

Problem

The original code used component.__dict__ to access component attributes during serialization. However, many frameworks (especially PyTorch) override __setattr__ to store attributes in custom data structures rather than self.__dict__. This caused:

  • KeyError when accessing attributes that exist but aren't in __dict__
  • Failed serialization of PyTorch models and other framework components
  • Inconsistent behavior across different class types

Solution

  • Replace __dict__ access with getattr() and hasattr(): Uses Python's standard attribute resolution mechanism
  • Cleaner parameter filtering: Explicitly exclude "self" along with "args" and "kwargs"
  • Universal compatibility: Works with any class regardless of internal attribute storage

Key Changes

# Before: Direct dictionary access (fragile)
attr_key = param if param in attrs.keys() else "_" + param
if attr_key in attrs.keys() and parameters[param].default != attrs[attr_key]:
    # Process attrs[attr_key]

# After: Standard attribute access (robust)
attr_key = param if param in attrs.keys() else "_" + param
attr_value = getattr(component, attr_key)
if attr_value is not None and parameters[param].default != attr_value:
    # Process attr_value

Impact

This change enables proper serialization of:

  • PyTorch nn.Module components
  • Any class that uses custom __setattr__ implementations
  • Standard Python classes (existing functionality preserved)

Without this fix, users cannot serialize FedJobs containing PyTorch models or other framework components, making the feature unusable for most deep learning workflows.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Quick tests passed locally by running ./runtest.sh.
  • In-line docstrings updated.
  • Documentation updated.

Copilot AI review requested due to automatic review settings August 7, 2025 05:29

This comment was marked as outdated.

Copy link
Collaborator

@yanchengnv yanchengnv left a comment

Choose a reason for hiding this comment

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

See my comments. Don't think we should continue when init arg fails to resolve.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a critical bug in the FedJob component serialization logic where direct __dict__ access fails for framework classes like PyTorch nn.Module that override attribute storage mechanisms.

  • Replaces fragile __dict__ access with robust getattr() calls for universal compatibility
  • Adds proper exception handling and cleaner parameter filtering
  • Enables serialization of PyTorch models and other framework components that use custom __setattr__ implementations

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@YuanTingHsieh
Copy link
Collaborator Author

/build

Copy link
Collaborator

@yanchengnv yanchengnv left a comment

Choose a reason for hiding this comment

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

See my comments. Looks like it may not be a good idea to use getattr after all.
Let's not rush to merge this.

@YuanTingHsieh
Copy link
Collaborator Author

yeah, we can wait after 2.7 branch out, then this can target at the main

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