Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix ModelDebugger.step to use only intermediate_outputs #2405

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Zerui18
Copy link

@Zerui18 Zerui18 commented Nov 21, 2024

Issue

Calling cto.coreml.experimental.linear_quantize_activations on a model with ct.ImageType output(s) fails at the model output because record_intermediate_output expects output_value to be a np.ndarray but the model's final output is PIL.Image.

Ultimately, record_intermediate_output only expects intermediate outputs, not model outputs which could be of type PIL.Image.

Followup From

#2385

@jakesabathia2
Copy link
Collaborator

Thanks for the PR @Zerui18 ,
could you also include an unittest for this fix?

@yixingli-apple
Copy link
Collaborator

Some references for activation quantization unit tests (you could also put your unit test there):

References for getting a mlmodel with ImageType as output:

@Zerui18
Copy link
Author

Zerui18 commented Dec 12, 2024

@jakesabathia2 I've added a corresponding unit test and rebased both changes to be a single commit on top of the current head. Please help to review it, thank you.

jakesabathia2
jakesabathia2 previously approved these changes Dec 18, 2024
Copy link
Collaborator

@jakesabathia2 jakesabathia2 left a comment

Choose a reason for hiding this comment

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

@jakesabathia2
Copy link
Collaborator

@Zerui18 the CI is failing.
I am seeing this branch is based on an old commit:

commit 3d7bfc8281fcfca31ad5a06f7203f763af2a5462
Author: M_noAria <[email protected]>
Date:   Fri Nov 1 01:39:36 2024 +0800

    Enhance torch.zeros_like for GPU inference in dynamic shape (#2369)

Can you rebase on the latest origin/main ?

`record_intermediate_output` only expects intermediate outputs, not model outputs which could be of type `PIL.Image`. Also added corresponding test on `_get_activation_calibration_stats` which is currently the only user entrypoint to `ModelDebugger.step`.
@Zerui18
Copy link
Author

Zerui18 commented Dec 23, 2024

Hi @jakesabathia2, I have rebase my commit on the current head as requested! Please have a look again. Thanks!

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