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

Update platform description to new format #567

Merged

Conversation

gmlueck
Copy link
Contributor

@gmlueck gmlueck commented Jun 14, 2024

Update the section describing the platform class to the new format.

gmlueck added 2 commits June 13, 2024 16:41
Update the section describing the `platform` class to the new format.

_Effects:_ The [code]#selector# is called once for every <<root-device>> as
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the once? Who knows, maybe some implementation wants to do two passes on the list of rood devices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this in 1b1bde4 because I agree that this section does not currently say that the selector is called exactly once for each device.

However, I wonder if this is an oversight in the spec. Why wouldn't we want to guarantee that it is called exactly once for each root device? This could make a difference for a selector that maintains internal state across each call.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, but that seems more than editorial as, in theory, it may impact a state-full selector, as you said.

I don't have a strong opinion about this one, but I lean toward adding once :)

It doesn't seem like a big ask for an implementer to call their selector only once per device, and indeed, it may simplify the user's implementation of a selector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, I think these "new format" PRs will be more than pure editorial, so it should be OK to make some small improvements. I don't mind making this as a separate PR, though, if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a new PR would give others an opportunity to chime in, and we could merge this PR without waiting for the "once" resolution.

adoc/chapters/programming_interface.adoc Show resolved Hide resolved
adoc/chapters/programming_interface.adoc Show resolved Hide resolved
@TApplencourt
Copy link
Contributor

Minimal nitpicking questions

gmlueck added 2 commits June 25, 2024 17:48
Use the same formal parameter name `type` for all the `get_devices`
member functions.
The description of device selectors in the "Device selection" section
does not say that a selector is called exactly once for each root
device.  Change the wording of the `platform` selector constructor to be
consistent with this wording, leaving the number of times it is called
ambiguous.

We might want to reconsider this, though.  Why wouldn't we want to
require the selector to be called exactly once?
@tomdeakin
Copy link
Contributor

Discussion on calling selector more than once - likely in some implementations. Open PR to discuss.

Copy link
Contributor

@tomdeakin tomdeakin left a comment

Choose a reason for hiding this comment

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

LGTM in general, and I like the new style. Two nitpicks about adding hyperlinks for user-friendliness, and one wording change we should think about to make sure we're happy it's equivalent.

adoc/chapters/programming_interface.adoc Show resolved Hide resolved
adoc/chapters/programming_interface.adoc Show resolved Hide resolved
adoc/chapters/programming_interface.adoc Outdated Show resolved Hide resolved
Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

That looks good!

gmlueck added 2 commits July 25, 2024 10:38
Restore the wording that `get_platforms` returns "available" platforms.
Add a link from `platform::get_info` to the section defining the
platform information descriptors.
@gmlueck
Copy link
Contributor Author

gmlueck commented Jul 25, 2024

@psalz: Do you know why this is getting a failure in "Open CTS issue for spec changes / create-issue"? It seems like there are two questions:

  • Why is this workflow running at all? This PR was already open, and it seems like the workflow triggered when I pushed a new commit. I think it was not your intent to run this workflow on each commit to a PR.

  • What causes the workflow error? The error is "Input required and not supplied: app-id". It looks like the YML file defines app-id as:

    app-id: ${{ vars.SYCL_ISSUE_BOT_APP_ID }}
    

    Is the problem that the variable is not defined somehow?

@psalz
Copy link
Contributor

psalz commented Aug 1, 2024

  • Why is this workflow running at all? This PR was already open, and it seems like the workflow triggered when I pushed a new commit. I think it was not your intent to run this workflow on each commit to a PR.

Addressed in #597.

  • What causes the workflow error? [...] Is the problem that the variable is not defined somehow?

I'm not sure what's happening here. Maybe because the PR predates the variable being set it doesn't receive it..? We'll have to keep an eye on this one. Also addressed in #597.

@tomdeakin
Copy link
Contributor

WG approves to merge after rev 9 release.
@gmlueck please merge after rev 9 release as editor.

@gmlueck
Copy link
Contributor Author

gmlueck commented Aug 22, 2024

Rev 9 was released, so merging.

@gmlueck gmlueck merged commit 6652116 into KhronosGroup:SYCL-2020/master Aug 22, 2024
2 of 3 checks passed
@gmlueck gmlueck deleted the gmlueck/reformat-platform branch August 22, 2024 17:04
keryell pushed a commit that referenced this pull request Sep 10, 2024
Update platform description to new format
gmlueck added a commit that referenced this pull request Nov 7, 2024
Update platform description to new format

(cherry picked from commit 6652116)
gmlueck added a commit that referenced this pull request Nov 7, 2024
Update platform description to new format

(cherry picked from commit 6652116)
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.

5 participants