Skip to content

Support multi instance endorsement #434

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 39 commits into
base: main
Choose a base branch
from

Conversation

andrew-draper
Copy link
Collaborator

Fixes issue #415

Copy link
Collaborator

@nedmsmith nedmsmith left a comment

Choose a reason for hiding this comment

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

multiple

@andrew-draper andrew-draper marked this pull request as ready for review June 6, 2025 12:05
These values behave in a similar way when used in the `group` field.

In a condition, the `tagged-variable` is used to indicate that two or more `environment-map`s must have the same `instance` value.
In an addition, the `tagged-variable` value is used to indicate that the `environment-map`.`instance` field must be copied from the ACS-ECT which matched a condition within the same triple.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
In an addition, the `tagged-variable` value is used to indicate that the `environment-map`.`instance` field must be copied from the ACS-ECT which matched a condition within the same triple.
In an addition, the `tagged-variable` value is used to indicate that the `environment-map`.`instance` field must be copied from the ACS-ECT which matched a condition, to the newly added `addition` within the same triple.

Co-authored-by: Yogesh Deshpande <[email protected]>
Copy link
Collaborator

@thomas-fossati thomas-fossati left a comment

Choose a reason for hiding this comment

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

I understand the problem that this is trying to solve, and I believe that the technical proposal will achieve the desired outcome.
However, from a design perspective, I don't think extending the environment map is the right approach.
The idea behind environment-map extensibility is to allow new concrete identifiers to be added to cover use cases and technologies that we didn't anticipate.
Instead, this PR adds new semantics to the identifier space, opening up a whole new world in a relatively stable corner of the spec.
This is likely to have consequences that we haven't fully grasped.
For this reason, even though I am clearly in the minority, I'd advise against merging this and instead exploring solutions that either do not require touching core/stable parts or use different constructs altogether.
If we decide to merge it nevertheless, I would like it to be marked as an “experimental" solution to the problem rather than one that we present as "the solution”.
And this should also be clearly reflected in the presentation to the wider group in Madrid.

@andrew-draper
Copy link
Collaborator Author

andrew-draper commented Jul 7, 2025

Thomas' comment is tracked in issue #459

If there is a better approach then I see no problem with using that approach instead of this one.

@yogeshbdeshpande
Copy link
Collaborator

@andrew-draper : Given that we need sufficient work to explain the clear steps of Conditions and Exit for the Algorithm as in the issue #458
How about we discuss some ideas in the coming CoRIM Meeting, this week and prepare a better proposal in the coming two weeks so that we can work on it at the Hackathon and bring it on the table for RATS session in Madrid ?

This gives us better time to work on an Alternative proposal and discuss face to face with other CoRIM Design team members.

Given your familiarity with the subject, in the background please work on a Clear Algorithm and then what is the best type
(other than instance ) to replace it, can be a simple search/replace in the PR to use a new code point example: measurement-values-map to deal with and progress this issue!

Copy link
Member

@henkbirkholz henkbirkholz left a comment

Choose a reason for hiding this comment

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

Rechecking after Thomas' well laid out "stability" argument #434 (review)

@henkbirkholz
Copy link
Member

The English text seems to be too vague about the problem statement. Additionally, I am having a hard time grasping "algorithm issues". This seems to be too hot of a potato to merge.

@andrew-draper
Copy link
Collaborator Author

If there were issues with this then I think it would have been good to raise them before the last minute, and it would be good to see the alternative proposal.

If there was an alternative proposal then it might make sense to delay putting in this change until we can decide between the alternatives. Since there isn't an alternative proposal I would prefer to do what Thomas suggested earlier, which is to check this in and be prepared to change to a better version if that is available.

If we don't accept this change then my take is that we don't have a complete solution to this issue, and we are not ready for last call.

@yogeshbdeshpande
Copy link
Collaborator

yogeshbdeshpande commented Jul 7, 2025

If there were issues with this then I think it would have been good to raise them before the last minute, and it would be good to see the alternative proposal.

If there was an alternative proposal then it might make sense to delay putting in this change until we can decide between the alternatives. Since there isn't an alternative proposal I would prefer to do what Thomas suggested earlier, which is to check this in and be prepared to change to a better version if that is available.

If we don't accept this change then my take is that we don't have a complete solution to this issue, and we are not ready for last call.

Thanks, we have much more better understanding of your problem, we would like to help you make correct design choice. Current design could cause potential instability in an area which is bed rock of CoRIM.

Let us discuss about the WGLC aspects on Wednesday!

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.

7 participants