First pass at IAWG pass at pack/unpack chapter #449

dsolt · 2023-04-24T17:41:20Z

Signed-off-by: David Solt [email protected]

Signed-off-by: David Solt <[email protected]>

dsolt · 2023-04-24T18:55:22Z

Outstanding issues not addressed yet in the PR:

Use of calloc/free in macros and in PMIx_data_load, No means to free the data allocated by PMIx_Data_compress and decompress
“These errors cannot be detected during packing” - not sure what to do with this.
Text talks about resetting the unpack_ptr, but there is no macro to do that.
If more values exist in the buffer than can fit into the memory storage, then the function will unpack what it can fit into that location and return an error code indicating that the buffer was only partially unpacked.
Why PMIx_data_compress does not take a byte object
data_embed and data_load names could be improved. (copy and move?)
should the whole chapter be moved later in the document since its not really core pmix functionality

dsolt · 2024-04-11T18:39:49Z

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.

…minor grammar issue Signed-off-by: David Solt <[email protected]>

rhc54 · 2024-04-12T10:57:45Z

Chap_API_Data_Mgmt.tex

@@ -199,14 +199,16 @@ \subsection{\code{PMIx_Data_pack}}

 The pack function packs one or more values of a specified type into the specified buffer.  The buffer must have already been
 initialized via the \refmacro{PMIX_DATA_BUFFER_CREATE} or \refmacro{PMIX_DATA_BUFFER_CONSTRUCT}
-macros --- otherwise, \refapi{PMIx_Data_pack} will return an error.
+macros --- otherwise, \refapi{PMIx_Data_pack} will return an error.  


Errr...that may not be true as there is no way for us to detect whether or not the buffer struct was initialized. It could just contain garbage.

rhc54 · 2024-04-12T10:58:17Z

Chap_API_Data_Mgmt.tex

-Note that any data to be packed that is not hard type cast (i.e.,
-not type cast to a specific size) may lose precision when unpacked
+Note that packing data using a type that 
+does not explicitly specifies its size may


specifies should be specify

rhc54 · 2024-04-12T11:00:38Z

Chap_API_Data_Mgmt.tex

@@ -243,11 +245,12 @@ \subsection{\code{PMIx_Data_unpack}}


 \begin{arglist}
-\argin{source}{Pointer to a \refstruct{pmix_proc_t} structure containing the nspace/rank of the process that packed the provided buffer. A NULL value may be used to indicate that the source is based on the same \ac{PMIx} version as the caller. Note that only the source's nspace is relevant. (handle)}
+\argin{source}{Pointer to a \refstruct{pmix_proc_t} structure containing the description of the process that packed the provided buffer. A NULL value may be used to indicate that the source is based on the same \ac{PMIx} version as the caller. 


Just a little confusing terminology - elsewhere, we refer to a pmix_proc_t as being the "process identifier", not a "description of the process". A description of the process sounds like a much larger set of information - e.g., where it is located, what executable is involved, etc.

rhc54 · 2024-04-12T11:04:10Z

Chap_API_Data_Mgmt.tex

-
-Note that it is possible for the buffer to be corrupted and that \ac{PMIx} will \textit{think} there is a proper variable type at the beginning of an unpack region --- but that the value is bogus (e.g., just a byte field in a string array that so happens to have a value that matches the specified data type flag). Therefore, the data type error check is \textit{not} completely safe.
+The unpack function unpacks the next value (or values) from the given buffer. Providing an unsupported type flag 
+or specifying a data type that \textit{does not} match the type of the next item in the buffer will be reported as an error. 


Note that this is only true if the library was told to use fully described buffers. Otherwise, we have no idea what the actual type of the data is - we just unpack the bits and put them in the data object of the type you claimed they should be.

rhc54 · 2024-04-12T11:04:53Z

Chap_API_Data_Mgmt.tex

-Note that it is possible for the buffer to be corrupted and that \ac{PMIx} will \textit{think} there is a proper variable type at the beginning of an unpack region --- but that the value is bogus (e.g., just a byte field in a string array that so happens to have a value that matches the specified data type flag). Therefore, the data type error check is \textit{not} completely safe.
+The unpack function unpacks the next value (or values) from the given buffer. Providing an unsupported type flag 
+or specifying a data type that \textit{does not} match the type of the next item in the buffer will be reported as an error. 
+An attempt to read an uninitialized buffer or read beyond the end of the stored data held in the buffer will also return an error.


Again, the uninitialized buffer clause isn't really correct - we have no idea if the buffer was initialized or just contains random garbage.

rhc54 · 2024-04-12T11:05:30Z

Chap_API_Data_Mgmt.tex


 Unpacking values is a "nondestructive" process --- i.e., the values are not removed from the buffer. It is therefore possible for the caller to re-unpack a value from the same buffer by resetting the unpack_ptr.

 Warning: The caller is responsible for providing adequate memory storage for the requested data. The user must provide a parameter indicating the maximum number of values that can be unpacked into the allocated memory. If more values exist in the buffer than can fit into the memory storage, then the function will unpack what it can fit into that location and return an error code indicating that the buffer was only partially unpacked.

-Note that any data that was not hard type cast (i.e., not type cast to a specific size) when packed may lose precision when unpacked by a non-homogeneous recipient. \ac{PMIx} will do its best to deal with heterogeneity issues between the packer and unpacker in such cases. Sending a number larger than can be handled by the recipient will return an error code generated upon unpacking --- these errors cannot be detected during packing.
+Note that any data that is packed using a type that does not explicitly specifies its size may lose precision when unpacked by a non-homogeneous recipient. \ac{PMIx} will do its best to deal with heterogeneity issues between the packer and unpacker in such cases. Sending a number larger than can be handled by the recipient will return an error code generated upon unpacking --- these errors cannot be detected during packing.


Believe you modified this above to replace "number" with "value"?

dsolt · 2024-05-09T15:43:59Z

Chap_API_Data_Mgmt.tex

@@ -4,7 +4,7 @@
 \chapter{Data Packing and Unpacking}
 \label{chap:api_data_mgmt}

-\ac{PMIx} intentionally does not include support for internode communications in the standard, instead relying on its host \ac{SMS} environment to transfer any needed data and/or requests between nodes. These operations frequently involve PMIx-defined public data structures that include binary data. Many \ac{HPC} clusters are homogeneous, and so transferring the structures can be done rather simply. However, greater effort is required in heterogeneous environments to ensure binary data is correctly transferred. \ac{PMIx} buffer manipulation functions are provided for this purpose via standardized interfaces to ease adoption.
+\ac{PMIx} intentionally does not include support for internode communications in the standard, instead relying on its host \ac{SMS} environment to transfer any needed data and/or requests between nodes.  However, to support \ac{SMS} environments which must frequently transfer \ac{PMIx} data structures between nodes and client applications that need to store or transfer \ac{PMIx} data structures, \ac{PMIx} provides the \acs{API} presented in this chapter. These operations frequently involve PMIx-defined data structures that include data that may have different binary representations on different hosts. Many \ac{HPC} clusters are homogeneous, and so transferring the structures can be done rather simply. However, greater effort is required in heterogeneous environments to ensure binary data is correctly transferred. \ac{PMIx} buffer manipulation functions are provided for this purpose via standardized interfaces to ease adoption.



Want to discourage use by client code by saying more about how this is really intended only for communicating structures across server nodes.

Chap_API_Data_Mgmt.tex

dsolt · 2024-05-09T15:54:35Z

Chap_API_Data_Mgmt.tex

 Providing an unsupported type flag will likewise be reported as an error.

-Note that any data to be packed that is not hard type cast (i.e.,
-not type cast to a specific size) may lose precision when unpacked


double may on line 207/208. Specifies -> specify (see Ralph's comment)

also changed on line approx 275:

"Note that any data that is packed using a type that does not explicitly specify its size may lose precision when unpacked by a non-homogeneous recipient. \ac{PMIx} will do its best to deal with heterogeneity issues between the packer and unpacker in such cases. Sending a number larger than can be handled by the recipient will return an error code generated upon unpacking --- these errors cannot be detected during packing."

Chap_API_Data_Mgmt.tex

Signed-off-by: David Solt <[email protected]>

dsolt · 2024-07-04T01:43:55Z

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.

Signed-off-by: David G. Solt <[email protected]>

naughtont3 · 2024-07-18T20:28:07Z

PR449 passed 1st vote (w/o changes) at ASC 2024-Q3 meeting.

abouteiller · 2024-10-17T19:09:46Z

Passed 2nd vote at ASC 24Q4 so it is now accepted.

First pass at IAWG pass at pack/unpack chapter

5dd2f62

Signed-off-by: David Solt <[email protected]>

dsolt added WorkInProgress Work In Progress Clarification labels Apr 24, 2023

dsolt removed the WorkInProgress Work In Progress label Mar 25, 2024

dsolt added 2 commits April 11, 2024 13:42

Merge branch 'master' into issue_175_packing

a7240dd

Simplify explanation of compressing being implementation dependant + …

dd0acbb

…minor grammar issue Signed-off-by: David Solt <[email protected]>

dsolt force-pushed the issue_175_packing branch from 0d12b02 to dd0acbb Compare April 11, 2024 18:55

rhc54 reviewed Apr 12, 2024

View reviewed changes

abouteiller added the Eligible Eligible for consideration by ASC label Apr 12, 2024

abouteiller added this to the PMIx v5.1 Standard milestone Apr 12, 2024

dsolt commented May 9, 2024

View reviewed changes

abouteiller added Major Text Change Correction RFC Request for Comment and removed Eligible Eligible for consideration by ASC labels May 10, 2024

Changes from first attempted (failed) reading

a7f83fa

Signed-off-by: David Solt <[email protected]>

dsolt force-pushed the issue_175_packing branch from d86f3f4 to a7f83fa Compare July 4, 2024 01:43

abouteiller added the Eligible Eligible for consideration by ASC label Jul 11, 2024

Stray "i." characters and restore accidentally deleted '.'

2e5d76e

Signed-off-by: David G. Solt <[email protected]>

naughtont3 added the First Vote Passed ASC first vote passed label Jul 18, 2024

abouteiller added Accepted as Provisional ASC vote passed. Accepted as Provisional! and removed Eligible Eligible for consideration by ASC RFC Request for Comment labels Oct 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First pass at IAWG pass at pack/unpack chapter #449

First pass at IAWG pass at pack/unpack chapter #449

dsolt commented Apr 24, 2023

dsolt commented Apr 24, 2023 •

edited

Loading

dsolt commented Apr 11, 2024

rhc54 Apr 12, 2024

rhc54 Apr 12, 2024

rhc54 Apr 12, 2024

rhc54 Apr 12, 2024

rhc54 Apr 12, 2024

rhc54 Apr 12, 2024

dsolt May 9, 2024

dsolt May 9, 2024

dsolt Jun 24, 2024 •

edited

Loading

dsolt commented Jul 4, 2024

naughtont3 commented Jul 18, 2024

abouteiller commented Oct 17, 2024

First pass at IAWG pass at pack/unpack chapter #449

Are you sure you want to change the base?

First pass at IAWG pass at pack/unpack chapter #449

Conversation

dsolt commented Apr 24, 2023

dsolt commented Apr 24, 2023 • edited Loading

dsolt commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsolt Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

dsolt commented Jul 4, 2024

naughtont3 commented Jul 18, 2024

abouteiller commented Oct 17, 2024

dsolt commented Apr 24, 2023 •

edited

Loading

dsolt Jun 24, 2024 •

edited

Loading