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

rfc20: add optional execution.properties key to Rv1 #313

Merged
merged 2 commits into from
Mar 11, 2022

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Feb 25, 2022

Problem: Rv1 does not support a method for assigning properties to
resources, but simple property support is a near term requirement for
Flux.

Add an optional properties key to the Rv1 execution dictionary which
allows for the simple assignment of named properties to sets of ranks.

@grondo grondo requested a review from dongahn February 25, 2022 22:39
@dongahn
Copy link
Member

dongahn commented Feb 25, 2022

This looks good to me. Since other people may ask the same question that I asked on the other issue ticket, we can say to the effect that each execution target can be tagged with an arbitrary set of properties using this method? (i.e., overlapping rank sets can be used to specify different properties)?

@grondo
Copy link
Contributor Author

grondo commented Feb 25, 2022

Thanks @dongahn, I added "A rank SHALL be assigned all properties for which its ID appears in the designated idset"

@grondo
Copy link
Contributor Author

grondo commented Feb 28, 2022

I thought it might be good to reserve some characters that can not be used in property names, since in the future we may want to support boolean queries on the command line like --requires='(foo || bar) && baz'. Unsure if I used the correct RFC syntax for that, but here's a starting point anyway.

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

This seems fine to me. It sounds like @dongahn is on board?

I tried rewording the paragraph for clarity but I'm not sure I succeeded. Your call. It's fine as is IMHO.

spec_20.rst Outdated
Comment on lines 166 to 176
**properties**
If present, the ``properties`` key SHALL be a dictionary of property
names, the values of which SHALL be a set of ranks in RFC 22 **idset
format** to which to assign the named property. A rank SHALL be assigned
all properties for which its ID appears in the designated idset.
Property names SHALL be valid UTF-8, and MUST NOT contain the
following reserved characters:

::

! & ' " ^ ` | ( )

Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but I find myself rereading it multiple times to "get it". Not sure if this is any better. It just breaks things up a bit so there is less english to parse (IMHO). 🤷

properties
The optional properties key SHALL be a dictionary where each key maps a single property name to a RFC 22 idset string. The idset string SHALL represent a set of execution target ranks. A given execution target rank MAY appear in multiple property mappings. Property names SHALL be valid UTF-8, and MUST NOT contain the
following reserved characters:

Copy link
Member

Choose a reason for hiding this comment

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

@grondo: Can we add @ as a reserved character as well? Then, we state to the effect that the suffix string that appears after @ SHALL be a scheduler-specific string. Given amd-mi50@gpu, amd-mi50 SHALL be the property string, but the scheduler MAY use the gpu suffix to perform a scheduling optimization for the gpu resources of the corresponding ranks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@garlick: Thanks, your rewrite sounds better IMO.

@dongahn: I will add something to that effect for the @ character, though I won't lump it with the other reserved characters that are not allowed to appear in a property name.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the description def. should be its own thing. Are you thinking @ would still be listed as one of the reserved characters, though?

Copy link
Contributor Author

@grondo grondo Mar 11, 2022

Choose a reason for hiding this comment

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

I changed the "reserved" characters to "illegal" characters since that seems more apt, then separately "reserved" the @ character for scheduler specific use. Happy to iterate until we're all satisfied with the final wording.

Copy link
Member

Choose a reason for hiding this comment

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

Works for me.

grondo and others added 2 commits March 10, 2022 19:42
Problem: Rv1 does not support a method for assigning properties to
resources, but simple property support is a near term requirement for
Flux.

Add an optional properties key to the Rv1 execution dictionary which
allows for the simple assignment of named properties to sets of ranks.

Slightly restrict the set of valid characters in property names.

Reserve the '@' character and any suffix for scheduler-specific use.

Co-authored-by: Jim Garlick <[email protected]>
Problem: There is no defintion of how to encode job constraints in
a standardized format.

Introduce RFC 31: Job Constraints Specification, which includes a simple,
but extensible format for encoding job constraints, inspired by the
json-logic definition at https://jsonlogic.com.
@mergify mergify bot merged commit c12339c into flux-framework:master Mar 11, 2022
@grondo grondo deleted the rv1-properties branch March 11, 2022 03:55
@grondo
Copy link
Contributor Author

grondo commented Mar 11, 2022

Thanks!

grondo added a commit to grondo/flux-framework-rfc that referenced this pull request Mar 11, 2022
This reverts commit b721ba1.

RFC 31 was inadvertently committed in PR flux-framework#313. Revert that commit
so that it can be properly proposed in a new PR.
grondo added a commit to grondo/flux-framework-rfc that referenced this pull request Mar 11, 2022
This reverts commit b721ba1.

The commit adding RFC 31 was inadvertently merged in PR flux-framework#313.
Revert the commit so it can be properly proposed in its own PR.
grondo added a commit to grondo/flux-framework-rfc that referenced this pull request Mar 11, 2022
This reverts commit b721ba1.

The commit adding RFC 31 was inadvertently merged in PR flux-framework#313.
Revert the commit so it can be properly proposed in its own PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants