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

Initial commit for IAWG review of Process Management chapter #500

Merged
merged 7 commits into from
Jan 17, 2025
Merged

Initial commit for IAWG review of Process Management chapter #500

merged 7 commits into from
Jan 17, 2025

Conversation

dsolt
Copy link
Contributor

@dsolt dsolt commented Oct 2, 2023

No description provided.

@dsolt dsolt added the WorkInProgress Work In Progress label Oct 2, 2023
rhc54
rhc54 reviewed Oct 2, 2023
View reviewed changes
Copy link
Member

@rhc54 rhc54 left a comment

Choose a reason for hiding this comment

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

Hope the comments help clarify things.

Chap_API_Proc_Mgmt.tex Outdated Show resolved Hide resolved
Chap_API_Proc_Mgmt.tex Outdated Show resolved Hide resolved
Chap_API_Proc_Mgmt.tex Outdated Show resolved Hide resolved
Chap_API_Proc_Mgmt.tex Outdated Show resolved Hide resolved
Chap_API_Proc_Mgmt.tex Outdated
@@ -432,7 +448,7 @@ \subsection{Spawn attributes}
}
%
\declareAttribute{PMIX_INDEX_ARGV}{"pmix.indxargv"}{bool}{
Mark the \code{argv} with the rank of the process.
If set to true, will use the given name of the executable (\code{argv[0]}) as a base name and each rank will be invoked using the base name with the string "-<\emph{rank}>" appended to it, where \emph{rank} is the \ac{PMIx} rank of the process being invoked.
Copy link
Member

Choose a reason for hiding this comment

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

Should probably make clear that PMIx isn't doing this - we are passing an attribute to the host environment requesting that it do this. Whether or not it happens is completely up to the host.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is true for SO many attributes throughout the standard. I don't know if we want to get into the job of specifying for every attribute how it is used. Someone reading the standard just wants to know what the attribute does.

Copy link
Member

Choose a reason for hiding this comment

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

Just noting that the change adds language like "...will use...", but leaves hanging the question of "who" will use the name. If you want to clarify, it might be something like "host is requested to use...". Or you can split this list to indicate which attributes are passed to the host for execution. Not entirely sure of the best way. Just trying to help you in your quest to clarify exactly what we mean here, and not replace one confusion with another.

Chap_API_Proc_Mgmt.tex Outdated Show resolved Hide resolved
Chap_API_Proc_Mgmt.tex Outdated Show resolved Hide resolved
rhc54
rhc54 reviewed Mar 18, 2024
View reviewed changes
Copy link
Member

@rhc54 rhc54 left a comment

Choose a reason for hiding this comment

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

Just trying to help clarify - I think much of this has to do with clarifying which implementation (host or PMIx) your changes are talking about.

Chap_API_Proc_Mgmt.tex Outdated Show resolved Hide resolved
Chap_API_Proc_Mgmt.tex Outdated Show resolved Hide resolved
Chap_API_Proc_Mgmt.tex Outdated
@@ -432,7 +448,7 @@ \subsection{Spawn attributes}
}
%
\declareAttribute{PMIX_INDEX_ARGV}{"pmix.indxargv"}{bool}{
Mark the \code{argv} with the rank of the process.
If set to true, will use the given name of the executable (\code{argv[0]}) as a base name and each rank will be invoked using the base name with the string "-<\emph{rank}>" appended to it, where \emph{rank} is the \ac{PMIx} rank of the process being invoked.
Copy link
Member

Choose a reason for hiding this comment

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

Just noting that the change adds language like "...will use...", but leaves hanging the question of "who" will use the name. If you want to clarify, it might be something like "host is requested to use...". Or you can split this list to indicate which attributes are passed to the host for execution. Not entirely sure of the best way. Just trying to help you in your quest to clarify exactly what we mean here, and not replace one confusion with another.

Chap_API_Proc_Mgmt.tex Outdated Show resolved Hide resolved
@dsolt
Changes from PR feedback
278e1d4 
Signed-off-by: David Solt <dsolt@us.ibm.com>
@dsolt dsolt removed the WorkInProgress Work In Progress label Mar 25, 2024
@dsolt dsolt added the RFC Request for Comment label Apr 11, 2024
@dsolt
Copy link
Contributor Author

dsolt commented Apr 11, 2024

Please use emoji reactions ON THIS COMMENT to indicate your position on this proposal.

  • You do not need to vote on every proposal
  • If you have no opinion, don't vote - that is also useful data
  • If you've already commented on this issue, please still vote so
    we know your current thoughts
  • Not all proposals solve exactly the same problem, so we may end
    up accepting proposals that appear to have some overlap
    This is not a binding majority-rule vote, but it will be a very
    significant input into the corresponding ASC decision.

Here are the meanings for the emojis:

  • Hooray or Rocket: I support this so strongly that I
    want to be an advocate for it
  • Heart: I think this is an ideal solution
  • Thumbs up: I'd be happy with this solution
  • Confused: I'd rather we not do this, but I can tolerate it
  • Thumbs down: I'd be actively unhappy, and may even consider
    other technologies instead
    If you want to explain in more detail, feel free to add another
    comment, but please also vote on this comment.

@abouteiller abouteiller added the Eligible Eligible for consideration by ASC label Apr 12, 2024
@abouteiller abouteiller added this to the PMIx v5.1 Standard milestone Apr 12, 2024
abouteiller
abouteiller reviewed May 7, 2024
View reviewed changes
Chap_API_Proc_Mgmt.tex Outdated Show resolved Hide resolved
Chap_API_Proc_Mgmt.tex Show resolved Hide resolved
Chap_API_Proc_Mgmt.tex Outdated Show resolved Hide resolved
Chap_API_Proc_Mgmt.tex Show resolved Hide resolved
dsolt
dsolt commented May 7, 2024
View reviewed changes
Chap_API_Proc_Mgmt.tex Show resolved Hide resolved
Chap_API_Proc_Mgmt.tex
\item \refconst{PMIX_ERR_JOB_INSUFFICIENT_RESOURCES} Insufficient resources to spawn job
\item \refconst{PMIX_ERR_JOB_SYS_OP_FAILED} System library operation failed
\item \refconst{PMIX_ERR_JOB_WDIR_NOT_FOUND} Specified working directory not found
\item a non-zero \ac{PMIx} error constant indicating a reason for the request's failure.
\end{itemize}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have a pastrefconst type of macro that we could use here instead of duplicating descriptions (which can get out of sync). If not, can we add such a macro across the standard (not just this chapter)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not appear to have one. Looks like one should be pretty easy to create, but should be applied across all chapters?

Copy link
Contributor

Choose a reason for hiding this comment

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

tracking issue #512

Chap_API_Proc_Mgmt.tex Outdated Show resolved Hide resolved
Chap_API_Proc_Mgmt.tex Outdated Show resolved Hide resolved
Chap_API_Proc_Mgmt.tex Show resolved Hide resolved
Chap_API_Proc_Mgmt.tex Show resolved Hide resolved
dsolt added 2 commits May 9, 2024 08:02
@dsolt
Editorial changes from first reading
4e0cc54 
Added '.' to end of many refconst descriptions
Changed provide back to display for PMIX_DISPLAY_MAP
Changed adviceimpl to advicerm for how process info is shared between parent/child
Fixed incorrect cut/paste of PMIX_SUCCESS description
Fixed case where argin was accidentally duplicated

Signed-off-by: David Solt <dsolt@us.ibm.com>
@dsolt
Add more information about PMIX_INDEX_ARGV
d5b9137 
Signed-off-by: David Solt <dsolt@us.ibm.com>
@abouteiller abouteiller mentioned this pull request May 10, 2024
Open
@abouteiller abouteiller added First Vote Passed ASC first vote passed Major Text Change and removed RFC Request for Comment labels May 10, 2024
@naughtont3 naughtont3 added the Accepted as Stable ASC second vote passed. Accepted as Stable! label Jul 18, 2024
@naughtont3
Copy link
Contributor

PR500 passed the 2nd vote at ASC 2024-Q3 meeting.

@abouteiller abouteiller force-pushed the issue_175_process_management branch from 048bada to 92a2adf Compare January 17, 2025 17:36
@abouteiller abouteiller merged commit f1b0946 into pmix:master Jan 17, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhc54 rhc54 rhc54 left review comments

@abouteiller abouteiller abouteiller approved these changes

Assignees
No one assigned
Labels
Accepted as Stable ASC second vote passed. Accepted as Stable! Eligible Eligible for consideration by ASC First Vote Passed ASC first vote passed Major Text Change
Projects
None yet
Milestone
PMIx v5.1 Standard
Development

Successfully merging this pull request may close these issues.
4 participants
@dsolt @naughtont3 @rhc54 @abouteiller