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

romio: redefine MPI_DISPLACEMENT_CURRENT #7247

Merged
merged 2 commits into from
Jan 14, 2025
Merged

Conversation

hzhou
Copy link
Contributor

@hzhou hzhou commented Dec 20, 2024

Pull Request Description

It is quite odd to have -54278278 for MPI_DISPLACEMENT_CURRENT let users and developers wonder its reasons. In fact, I believe this is used as a sentinel dummy argument and an arbitary value should work.

The value -1 is as arbitrary as -54278278 and people won't wonder.

Author Checklist

  • Provide Description
    Particularly focus on why, not what. Reference background, issues, test failures, xfail entries, etc.
  • Commits Follow Good Practice
    Commits are self-contained and do not do two things at once.
    Commit message is of the form: module: short description
    Commit message explains what's in the commit.
  • Passes All Tests
    Whitespace checker. Warnings test. Additional tests via comments.
  • Contribution Agreement
    For non-Argonne authors, check contribution agreement.
    If necessary, request an explicit comment from your companies PR approval manager.

@hzhou hzhou marked this pull request as draft December 20, 2024 20:03
@hzhou hzhou force-pushed the 2412_DISPLACEMENT branch from 1cb7c44 to 7cefb98 Compare December 20, 2024 20:06
@hzhou
Copy link
Contributor Author

hzhou commented Dec 20, 2024

test:mpich/ch3/tcp

test:mpich/custom
config: mpi-abi

Somehow cvars such as MPIR_CVAR_ALLTOALLW_INTRA_ALGORITHM is not set properly with abi and the new testing framework. But the IO tests seems fine.

@hzhou hzhou force-pushed the 2412_DISPLACEMENT branch from 7cefb98 to 05c3ec9 Compare December 22, 2024 03:09
@hzhou
Copy link
Contributor Author

hzhou commented Dec 22, 2024

test:mpich/custom
config: mpi-abi

@@ -425,7 +425,7 @@ enum {
};

/* File Operation Constants */
#define MPI_DISPLACEMENT_CURRENT ((MPI_Offset)-54278278)
#define MPI_DISPLACEMENT_CURRENT ((MPI_Offset)-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Tests look OK with this value. Should be propose a change to https://github.com/mpi-forum/mpi-standard/pull/977 for next week's second vote?

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 am for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me drop the last commit (05c3ec9) and test. If test pass, it means we can maintain both MPICH ABI and whatever negative value MPI ABI will use.

@hzhou hzhou force-pushed the 2412_DISPLACEMENT branch from 05c3ec9 to 488b8f8 Compare January 3, 2025 20:12
@hzhou
Copy link
Contributor Author

hzhou commented Jan 3, 2025

test:mpich/custom
config: mpi-abi
✔️

@hzhou hzhou marked this pull request as ready for review January 3, 2025 20:12
@hzhou
Copy link
Contributor Author

hzhou commented Jan 3, 2025

test:mpich/ch3/tcp ✔️
test:mpich/ch4/ofi

@hzhou hzhou removed the do-not-merge label Jan 9, 2025
@hzhou hzhou requested a review from raffenet January 9, 2025 17:13
@dalcinl
Copy link
Contributor

dalcinl commented Jan 11, 2025

@raffenet @hzhou Could you please bump priority to the review & merge of this PR? mpi4py's MPI ABI testing requires the MPI ABI stubs header and MPICH's MPI ABI implementation in sync.

@hzhou hzhou force-pushed the 2412_DISPLACEMENT branch from 488b8f8 to fd918a7 Compare January 14, 2025 16:47
hzhou added 2 commits January 14, 2025 10:50
Otherwise, the MPI_T functions may not able to convert
builtin datatypes.
@hzhou hzhou force-pushed the 2412_DISPLACEMENT branch from fd918a7 to da3a9b1 Compare January 14, 2025 16:50
@hzhou hzhou merged commit 9f67af6 into pmodels:main Jan 14, 2025
4 checks passed
@hzhou hzhou deleted the 2412_DISPLACEMENT branch January 14, 2025 16:59
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.

3 participants