Skip to content

Make hooks seedable and added instance-level properties - #396

Merged
ntgbaoo merged 9 commits into
mainfrom
381-InstanceLevelHookResolution
Apr 14, 2026
Merged

Make hooks seedable and added instance-level properties#396
ntgbaoo merged 9 commits into
mainfrom
381-InstanceLevelHookResolution

Conversation

@ntgbaoo

@ntgbaoo ntgbaoo commented Feb 16, 2026

Copy link
Copy Markdown
Member

Summary / Description

This PR covered:

Related Issues: #381

Type of Change

  • Bug fix
  • New feature
  • Breaking Change
  • Refactoring
  • Documentation update

Test Evidence

Describe how this PR has been tested.

  • Unit tests
  • Integration tests
  • Performance tests

Questions / Discussion Points

I will add this function to BaseDGHook:

    def add_attribute_to_batch(self, batch: DGBatch ,name: str, value: Any) -> None:
        """
        Add a new attribute to providede batch.

        If `id` is specified, the new attribute name will be appended with the given `id` as a suffix.
        """
        if self.id:
            name = f'{name}_{self.id}'
        setattr(batch, name, value)

I will refactor our code so we use this method every time we add a new attribute to the batch

This is a draft, and unit tests will be added to improve codecov.

@ntgbaoo ntgbaoo self-assigned this Feb 16, 2026
@ntgbaoo
ntgbaoo marked this pull request as draft February 16, 2026 04:40
@ntgbaoo
ntgbaoo marked this pull request as ready for review February 16, 2026 04:40
@codecov

codecov Bot commented Feb 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.33333% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
tgm/hooks/negatives.py 95.77% 3 Missing ⚠️
tgm/hooks/dedup.py 95.83% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@Jacob-Chmura Jacob-Chmura left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've done a pass and left some thoughts on the design. The directions looks good but there are some details to iron out.

Comment thread tgm/hooks/dedup.py Outdated
Comment thread tgm/hooks/dedup.py Outdated
Comment thread tgm/hooks/base.py Outdated
Comment thread tgm/hooks/dedup.py
Comment thread tgm/hooks/dedup.py Outdated
@ntgbaoo
ntgbaoo requested a review from Jacob-Chmura March 23, 2026 14:33
@ntgbaoo ntgbaoo changed the title Made requires and produces to instance level variables Make hooks seedable and added instance-level properties Mar 23, 2026
@ntgbaoo ntgbaoo modified the milestone: 381 Mar 23, 2026
@ntgbaoo ntgbaoo linked an issue Mar 23, 2026 that may be closed by this pull request
@ntgbaoo ntgbaoo added this to the Beta Release 3 milestone Mar 23, 2026

@shenyangHuang shenyangHuang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall looks good, some comments to address

Comment thread tgm/hooks/base.py Outdated
Comment thread tgm/hooks/base.py Outdated
Comment thread tgm/hooks/base.py
@ntgbaoo
ntgbaoo requested a review from shenyangHuang March 30, 2026 21:23

@shenyangHuang shenyangHuang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks good, thanks Bao

@ntgbaoo
ntgbaoo merged commit 32f7bfc into main Apr 14, 2026
7 checks passed
@ntgbaoo
ntgbaoo deleted the 381-InstanceLevelHookResolution branch June 8, 2026 01:07
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.

Instance-Level Hook Resolution, with seedable requirements

3 participants