Skip to content
New issue

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

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

Already on GitHub? # to your account

Formalize annotated name parsing in DASH. #473

Merged
merged 2 commits into from
Dec 10, 2023

Conversation

r12f
Copy link
Collaborator

@r12f r12f commented Dec 9, 2023

Use a normalized way to parse the annotated name in p4 files.

This change will not change any generated files as shown below.

r12f@r12f-dl380:~/data/code/sonic/DASH/dash-pipeline
$ diff SAI/SAI/experimental/ ~/data/code/sonic/DASH-exp/dash-pipeline/SAI/SAI/experimental/

r12f@r12f-dl380:~/data/code/sonic/DASH/dash-pipeline
$ diff SAI/lib ~/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib

@r12f r12f requested a review from marian-pritsak December 9, 2023 19:42
@chrispsommers
Copy link
Collaborator

Hi @r12f I appreciate this work, but it might be a step in the wrong direction. I am wondering if we really ought to deprecate the P4 entity naming convention e.g. "@name(x|y)" entirely and follow-through on #276 where we agreed that long-term, we should not use a naming convention at all, instead use explicit annotations e.g. "@Sai[a=b,x=y]".

@marian-pritsak took initial steps towards this in #358. See https://github.com/sonic-net/DASH/blob/main/dash-pipeline/bmv2/README.md#p4-annotations-for-sai-code-generation also.

That being said, if this is just cleanup and uniformization, I see no objections to it, but it is kind of perpetuating a convention we'd rather abandon.

Please take a look at the conversation tail above and let us know what you think!

@r12f r12f requested a review from kcudnik December 9, 2023 19:53
@r12f
Copy link
Collaborator Author

r12f commented Dec 9, 2023

Hi @chrispsommers , yes. I totally agree with you! This is just a clean up work in case things get even worse and we should definitely move the annotation to use @Sai instead.

And thanks for sharing the docs! Once this is done, I will try to make the work to move these annotations to @Sai as the next step.

@chrispsommers
Copy link
Collaborator

in case things get even worse
LOL!

OK, with that in mind, I am in favor of this. Thanks for considering taking the annotations in the right direction when time allows. We all appreciate the energy you are bringing to this project!

@chrispsommers chrispsommers self-requested a review December 9, 2023 20:01
@r12f
Copy link
Collaborator Author

r12f commented Dec 9, 2023

np at all! and I also really appreciated reviewing this PR in the weekend too!

@r12f
Copy link
Collaborator Author

r12f commented Dec 9, 2023

in case things get even worse
LOL!

I cannot believe myself used almost half of day just trying to sort the naming out..... XD

@r12f r12f force-pushed the user/r12f/sai-name branch from 6f2bd11 to 9f158c3 Compare December 10, 2023 20:21
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants