Skip to content

message_envelope and recv less bytes#10

Closed
Quentin-Anthony wants to merge 23 commits intoQuentin-Anthony:mainfrom
jadenvv:main
Closed

message_envelope and recv less bytes#10
Quentin-Anthony wants to merge 23 commits intoQuentin-Anthony:mainfrom
jadenvv:main

Conversation

@Quentin-Anthony
Copy link
Copy Markdown
Owner

Summary

Added a Message Envelope (with tag and size) and created the implementation of MPI_ANY_SOURCE

implementation details

-- In the message envelope I thought it was redundant to create a destination part in it since its implied. Still need to add tags when checking to receive data.
-- Maybe there's a better way to implement MPI_ANY_SOURCE other than using poll()
-- I added the size of the buffer in the message envelope in order for nanoMPI recv to allow for smaller buffers on the sending side

Improvements

-- adding rendezvous tag matching

See #5 for previous discussion

@nsarka
Copy link
Copy Markdown
Collaborator

nsarka commented Jul 21, 2025

@jadenvv It turns out adding the commit history prior to the first commit in this repo is hard to do without messing everything up, so I undid it and @Quentin-Anthony reopened your PR here

@Quentin-Anthony
Copy link
Copy Markdown
Owner Author

@jadenvv -- Can you revert fd0c7d2bd43534e842acca510b9313e825b6f4a5 so that we can merge? Or you could add me as a collaborator on your repo so that I can edit it again (I can't since I opened this PR, not you). Those formatting changes are no longer needed after #9

Sorry again for the git history chaos! @nsarka and I were trying to correct some earlier commits, and we had to force push to get things back in order.

@jadenvv
Copy link
Copy Markdown

jadenvv commented Jul 21, 2025

@jadenvv -- Can you revert fd0c7d2bd43534e842acca510b9313e825b6f4a5 so that we can merge? Or you could add me as a collaborator on your repo so that I can edit it again (I can't since I opened this PR, not you). Those formatting changes are no longer needed after #9

Sorry again for the git history chaos! @nsarka and I were trying to correct some earlier commits, and we had to force push to get things back in order.

Yep no problem!

@jadenvv
Copy link
Copy Markdown

jadenvv commented Jul 21, 2025

I added a lot but most of it doesn't work right now. And just wanted to go over a couple of things I did in my most recent commit.

  • I added a queue for unexpected sends(send_tag != dest_tag) but there is some logic issues with this that I'll fix when I wake up. But please check the queue logic itself it gets pretty tedious in C. And it would be nice if we could create a way to test that in the rank/thread which has an incompatible tag doesn't block. For instance, if we have say 3 sends(with respective tags 1,2..) all going to the same destination and a single recv, for say 2. But it goes in the order 1,3,2 the current code will block at 1(I think, honestly, I'm really tired and will edit this in the morning).
  • I also added a tag parameter for nanompi_socket_recv because we want to do the tag verification on the backend and then if it doesn't match we enqueue it
  • And in the current version of poll, I don't know if it works on your guy's end, but I don't think its quite reliable. Although, I'll look over it again tomorrow
  • Lastly, I don't know if it was from the linter, but the order of the header files changed in one of the versions and it messed up compilation.

Alright, that's it for tonight. Good night!

@nsarka
Copy link
Copy Markdown
Collaborator

nsarka commented Jul 23, 2025

the queue logic itself it gets pretty tedious in C

I haven't checked any logic yet, but we can always use C++ and extern "C" in any needed code from the rest of the project!

@jadenvv
Copy link
Copy Markdown

jadenvv commented Jul 23, 2025

Honestly, that sounds like a better decision later down the road. btw @nsarka accept the collaborator invite so you are able to merge an earlier commit like the one Quentin pointed out. And we can close this PR I'll just create another one for the queue.

@nsarka
Copy link
Copy Markdown
Collaborator

nsarka commented Jul 23, 2025

Ok, closing this PR then. In the future it's good practice to mark PRs not ready for review as a draft. Once they're ready for review double check it follows these rules (which are standard, at least in my experience making PRs at NVIDIA):

  • Unused code is deleted, comments should be just for explaining what code does
  • Code is formatted to match the rest of the repo
  • Each PR handles one thing at a time. For example, the message envelope should be one PR, handling tag matching another, formatting another, etc.
  • If it can be helped, PRs are better if they are under 200-300 lines. Any more than that and they're at risk of never being looked it, or worse, approved and merged without any scrutiny

@nsarka nsarka closed this Jul 23, 2025
@nsarka
Copy link
Copy Markdown
Collaborator

nsarka commented Jul 23, 2025

Another thing that's helpful when working from a fork--make branches in your repo and then open PRs to merge your branches. That way you can manage multiple at once, and merging changes from upstream is just hitting the "sync fork" button on github, then git merge main from the command line while on your branch

@jadenvv
Copy link
Copy Markdown

jadenvv commented Jul 23, 2025

Alright I'll definitely keep that in mind for future PRs. Thanks.

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