Skip to content

Conversation

@glistening
Copy link
Contributor

It adds forward's input recorder.

TICO-DCO-1.0-Signed-off-by: Sanggyu Lee sg5.lee@samsung.com

Related: #207
Draft: #217

It adds forward's input recorder.

TICO-DCO-1.0-Signed-off-by: Sanggyu Lee <sg5.lee@samsung.com>
@glistening glistening requested a review from a team July 28, 2025 23:24
self.args_names = [
name for name in sig.parameters.keys() if name not in ("self", "kwargs")
]
self.captured_input = ()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.captured_input = ()
self.captured_input = None

How about initializing this with None?
Because, in some rare case, captured input could be void...

Copy link
Contributor

Choose a reason for hiding this comment

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

@glistening Could you check this..?

Co-authored-by: Dayoung Lee <dayoung.lee@samsung.com>
Comment on lines 69 to 70
args_dict = dict(zip(self.args_names, args))
args_dict.update(kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this works!

args_dict = dict(sig.bind(*args, **kwargs).arguments)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I am not sure it is perfectly equivalent to my original code. I have to refer to sig.bind and arguments and so on. Even it is equivalent, I found no benefit of using this API. Is there any advantage to replace 2 lines of straightforward code to your suggestion?

Copy link
Contributor Author

@glistening glistening Jul 29, 2025

Choose a reason for hiding this comment

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

I've searched the benefits of your way. Your suggestion works better in several points: 1) when the method has default value, ...

I will update as you suggested after checking more.

Copy link
Contributor Author

@glistening glistening Jul 29, 2025

Choose a reason for hiding this comment

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

@dayo09 I am investigating sig.bind stuffs.

Your suggestion binds only the explicitly passed arguments ( len() = 7 ), not all arguments.

{
 'input_ids': tensor([[    1, 21075,  7727,   550,   260, 12584, 31843,     2,     2,     2,  2]]), 
 'attention_mask': tensor([[1, 1, 1, 1, 1, 1, 1, 0, 0, 0, ..., 0, 0, 0, 0, 0]]), 
 'past_key_values': DynamicCache(), 
 'inputs_embeds': None, 
 'use_cache': True, 
 'return_dict': True, 
 'cache_position': tensor([ 0,  1,  2,  3,  4,  5,  6, ... 28, 29, 30, 31])
}

while I wanted arg_dicts has the whole positional arguments ( len() = 12 ) exactly in same order. 1

def forward(
    self,
    input_ids: Optional[torch.LongTensor] = None,
    attention_mask: Optional[torch.Tensor] = None,
    position_ids: Optional[torch.LongTensor] = None,
    past_key_values: Optional[Cache] = None,
    inputs_embeds: Optional[torch.FloatTensor] = None,
    labels: Optional[torch.LongTensor] = None,
    use_cache: Optional[bool] = None,
    output_attentions: Optional[bool] = None,
    output_hidden_states: Optional[bool] = None,
    cache_position: Optional[torch.LongTensor] = None,
    logits_to_keep: Union[int, torch.Tensor] = 0,
    **kwargs: Unpack[KwargsForCausalLM],
) -> CausalLMOutputWithPast:

I have to call bound.apply_defaults()

bound = self.sig.bind(*args, **kwargs)
bound.apply_defaults()
args_dict = dict(bound.arguments)

Now, I will check whether it works better than my original code.
I guess it will work better for non-None default arg, which did not happen in my target models. I will modify the target model and see what happens.

Footnotes

  1. If the whole arguments is not passed, torch.export complains that the number of arguments is different.

Copy link
Contributor

Choose a reason for hiding this comment

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

@glistening I don't have a strong preference - go with what works for you 😄

Copy link
Contributor

@dayo09 dayo09 left a comment

Choose a reason for hiding this comment

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

LGTM Thanks!

Copy link
Contributor

@mhs4670go mhs4670go left a comment

Choose a reason for hiding this comment

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

LGTM

@mhs4670go mhs4670go merged commit 7c80975 into Samsung:main Jul 29, 2025
6 checks passed
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.

3 participants