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.

Sign up for GitHub

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

Jump to bottom

Adding output schemas #32

Merged
merged 13 commits into from
Apr 6, 2023
Merged

Adding output schemas #32

merged 13 commits into from
Apr 6, 2023

Conversation

idavis
Copy link
Collaborator

@idavis idavis commented Feb 23, 2023 •
edited
Loading

This PR defines the output schemas for labeled and unlabeled schemas.

The format also defines what the output should look like for output recording functions for int, double, and bool which are not currently specified in the base profile. We can remove these functions from the schemas or keep them noting that they are forward looking definitions.

This spec, along with the base profile will have to be updated as we address the opaque pointer updates to LLVM IR.

@github-actions
Copy link

github-actions bot commented Feb 23, 2023 •
edited
Loading

CLA Assistant Lite bot All Contributors have signed the CLA.

@idavis
Copy link
Collaborator Author

idavis commented Feb 23, 2023

I have read the Contributor License Agreement and I hereby accept the Terms.

@idavis
Copy link
Collaborator Author

idavis commented Feb 27, 2023

Hi @bettinaheim, could you please review the schema definitions?

@idavis
Copy link
Collaborator Author

idavis commented Mar 2, 2023

See #23

@idavis
Copy link
Collaborator Author

idavis commented Mar 8, 2023

@bettinaheim /bump

@idavis
Copy link
Collaborator Author

idavis commented Mar 19, 2023

/CC @qci-amos @kalzoo

qci-amos
qci-amos reviewed Mar 20, 2023
View reviewed changes
Copy link

@qci-amos qci-amos left a comment

Choose a reason for hiding this comment

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

lgtm, this all appears consistent with what my understanding is for how this should go, no surprises! I've just added some thoughts as I read through it where come additional commentary/clarifications would be useful to some readers.

specification/under_development/output_schemas/Labeled_And_Async.md Show resolved Hide resolved
specification/under_development/output_schemas/Labeled_And_Async.md Show resolved Hide resolved
specification/under_development/output_schemas/No_Labels_And_Ordered.md Outdated Show resolved Hide resolved
specification/under_development/output_schemas/No_Labels_And_Ordered.md Outdated Show resolved Hide resolved
specification/under_development/output_schemas/Labeled_And_Async.md Outdated Show resolved Hide resolved
specification/under_development/output_schemas/Labeled_And_Async.md Show resolved Hide resolved
specification/under_development/output_schemas/No_Labels_And_Ordered.md Show resolved Hide resolved
specification/under_development/output_schemas/Labeled_And_Async.md Show resolved Hide resolved
@spignorel spignorel requested a review from dan1pal March 20, 2023 20:10
@idavis
Copy link
Collaborator Author

idavis commented Mar 30, 2023

While I understand why the method signatures are specialized, I wouldn't mind a sentence explaining why RESULT and BOOL outputs aren't emitted as INT.

Added a comment in Notes For Implementors

qci-amos
qci-amos reviewed Apr 4, 2023
View reviewed changes
Copy link

@qci-amos qci-amos left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I like the new grammar and the notes docs, I think they really help!

It's not letting me resolve conversations, so you'll have to do that. There's only one of my original ones which I don't think was answered yet and which looks like it requires a fix or a clarification. The other comments here from me are mostly just curiosity to do with what you want.

specification/under_development/output_schemas/Grammars.md Show resolved Hide resolved
specification/under_development/output_schemas/Grammars.md Outdated Show resolved Hide resolved
specification/under_development/output_schemas/No_Labels_And_Ordered.md Outdated Show resolved Hide resolved
specification/under_development/output_schemas/No_Labels_And_Ordered.md Outdated Show resolved Hide resolved
qci-amos
qci-amos approved these changes Apr 5, 2023
View reviewed changes
Copy link

@qci-amos qci-amos left a comment

Choose a reason for hiding this comment

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

lgtm @idavis!

one last thought is that some people may be interested in being able to read a log themselves and extract some kind of data structure, perhaps for their own unit testing. if you have any open source code for this already, it would be nice to include a link to that somewhere.

@idavis
Copy link
Collaborator Author

idavis commented Apr 5, 2023

Thanks @qci-amos . I don't know of any OSS parsing utils for the output formats. Might be good to discuss at the next meeting. I don't have permissions to merge, so someone else will have to merge this PR.

@idavis idavis merged commit 007e98a into qir-alliance:spec_update Apr 6, 2023
@idavis idavis deleted the iadavis/ouput-formats branch April 6, 2023 15:54
@github-actions github-actions bot locked and limited conversation to collaborators Apr 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@peter-campora peter-campora peter-campora left review comments

@dan1pal dan1pal dan1pal approved these changes

@qci-amos qci-amos qci-amos approved these changes

@cesarzc cesarzc cesarzc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@idavis @peter-campora @cesarzc @qci-amos @dan1pal