Skip to content

[circle-resizer] Add Shape parser #15082

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mbencer
Copy link
Contributor

@mbencer mbencer commented Apr 8, 2025

This commit adds the shapes representation and capabilities to parse string inputs into Shapes objects.

ONE-DCO-1.0-Signed-off-by: Mateusz Bencer m.bencer@partner.samsung.com

Issue: #14791
Draft: #14727

This commit adds the shapes representation and capabilities to parse string inputs into Shapes objects.

ONE-DCO-1.0-Signed-off-by: Mateusz Bencer m.bencer@partner.samsung.com
@mbencer mbencer requested a review from a team April 8, 2025 11:56
@mbencer mbencer requested review from jinevening and a team April 10, 2025 09:06
@mbencer mbencer added the PR/NO MERGE Please don't merge. I'm still working on this :) label Apr 10, 2025
jinevening
jinevening previously approved these changes Apr 11, 2025
Copy link
Contributor

@jinevening jinevening left a comment

Choose a reason for hiding this comment

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

LGTM

@mbencer
Copy link
Contributor Author

mbencer commented Apr 11, 2025

LGTM

Thank you very much but please give me a moment (before merging) to add doc+unit tests to Shape class ;-)

@jinevening
Copy link
Contributor

Ah, sorry for the impatience. Please let me know when you're ready.

@jinevening jinevening self-requested a review April 11, 2025 07:08
@mbencer mbencer removed the PR/NO MERGE Please don't merge. I'm still working on this :) label Apr 11, 2025
@mbencer
Copy link
Contributor Author

mbencer commented Apr 11, 2025

Ah, sorry for the impatience. Please let me know when you're ready.

@jinevening I believe that's the implementation is ready now ;-)

@mbencer mbencer requested a review from a team April 11, 2025 12:15
@mbencer mbencer requested a review from jinevening April 14, 2025 06:23
@mbencer mbencer requested a review from a team April 14, 2025 06:23
Copy link
Contributor

@jinevening jinevening left a comment

Choose a reason for hiding this comment

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

LGTM

@jinevening jinevening requested a review from seanshpark April 15, 2025 03:52
@seanshpark seanshpark removed their request for review April 15, 2025 03:53
@seanshpark
Copy link
Contributor

sorry but too huge to review

@jinevening
Copy link
Contributor

@seanshpark I understand. This PR got bigger as @mbencer and I discuss how to handle scalar type. Can we merge as-is with 1 approval or do you want to review splitted PRs?

@seanshpark
Copy link
Contributor

Can we merge as-is with 1 approval or do you want to review splitted PRs?

With the discussion issue, my questions wasn't resolved and still I am not clear with this module's direction or usage scenario with onecc and TICO, but the PR was posted and landed.
Please go as you wish as I understand you've described usage case with TICO.

@mbencer
Copy link
Contributor Author

mbencer commented Apr 15, 2025

@seanshpark

sorry but too huge to review

there is a pretty convenient way now to split the PR into Shape part and parser part. I can prepare such a version

With the discussion issue, my questions wasn't resolved and still I am not clear with this module's direction or usage scenario with onecc and TICO, but the PR was posted and landed.
Please go as you wish as I understand you've described usage case with TICO.

I've collected all(I hope) requests/questions around resizer in #14791 (comment). In particular I agree to stop the idea of a separate python package for resizer. Integration with onecc ecosystem is shown in the draft.
The usage in the test pipeline in TICO I've tried to show in this draft.
Please let me known if you have other questions/requests ;-)

@jinevening
Copy link
Contributor

@seanshpark I landed the entrypoint PR because I thought you agreed to develop circle-resizer as a command line tool (based on the reaction to my comment :) ). There was some remaining discussion on python packaging, but AFAIK that issue is independent with development of circle-resizer.

If you disagree to develop circle-resizer even as a command line tool, please let me know. Then we may need to talk about how to support multiple shapes for generative models.

@mbencer mbencer added the PR/NO MERGE Please don't merge. I'm still working on this :) label Apr 15, 2025
@mbencer
Copy link
Contributor Author

mbencer commented Apr 15, 2025

sorry but too huge to review

@seanshpark @jinevening Please review a smaller piece of this PR - adding Shape representation at first. After #15165 merge I'll synchronize this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/NO MERGE Please don't merge. I'm still working on this :)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants