Skip to content

Conversation

@aditya0by0
Copy link
Member

Currently, in the GraphRepository, the setup method is overridden to call _setup_properties() after the processed data is available:

def setup(self, **kwargs):
    super().setup(keep_reader=True, **kwargs)
    self._setup_properties()

    self.reader.on_finish()

This override introduces a couple of issues:

  1. Bypassing the setup flag:
    The base class uses _setup_data_flag to prevent multiple invocations of the setup() method:

    def setup(self, *args, **kwargs) -> None:
        if self._setup_data_flag != 1:
            return
        self._setup_data_flag += 1
        ...

    Overriding setup() in the derived class causes this mechanism to be bypassed or duplicated unintentionally.

  2. Manual control over reader finalization:
    To prevent reader.on_finish() from executing before _setup_properties() is complete, the derived class needs to manually pass keep_reader=True to the base setup() method. This adds unnecessary complexity and tight coupling between the base and derived classes.


Proposed Solution: Introduce _after_setup() hook in the base class

To resolve these issues cleanly, introduce an _after_setup(**kwargs) method in the base class. This hook would be called after data preprocessing and flag updates are completed:

Base class:

def setup(self, *args, **kwargs) -> None:
    if self._setup_data_flag != 1:
        return

    self._setup_data_flag += 1
    ...
    self._after_setup(**kwargs)

def _after_setup(self, **kwargs):
    self.reader.on_finish()
    self._set_processed_data_props()

GraphRepository subclass:

def _after_setup(self, **kwargs):
    self._setup_properties()
    super()._after_setup(**kwargs)

This change enables:

  • Cleaner and safer extension of the setup process.
  • Proper use of the base class flag mechanism.
  • Decoupling of reader finalization from the timing of _setup_properties().

@aditya0by0 aditya0by0 self-assigned this May 31, 2025
@aditya0by0 aditya0by0 requested a review from sfluegel05 May 31, 2025 18:23
@aditya0by0 aditya0by0 changed the title Issue: Improve setup override logic for processed property handling in GraphRepository Improve setup override logic for processed property handling in GraphRepository May 31, 2025
@aditya0by0 aditya0by0 added bug Something isn't working bug:fix priority: high labels May 31, 2025
@aditya0by0
Copy link
Member Author

@sfluegel05 Please review and merge

@sfluegel05 sfluegel05 marked this pull request as ready for review June 3, 2025 14:04
Copy link
Collaborator

@sfluegel05 sfluegel05 left a comment

Choose a reason for hiding this comment

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

As we have discussed earlier, this is definitely an improvement and a cleaner solution for the graph repository (where the after_setup is extended)

@sfluegel05 sfluegel05 merged commit f74b57c into dev Jun 3, 2025
6 checks passed
@aditya0by0 aditya0by0 deleted the fix/after_setup_hook branch June 4, 2025 12:31
aditya0by0 added a commit to ChEB-AI/python-chebai-graph that referenced this pull request Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug:fix bug Something isn't working priority: high

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants