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

number_distributed_tags: non-set, non-sorted numbering #469

Merged
merged 8 commits into from
Feb 6, 2024
Merged

number_distributed_tags: non-set, non-sorted numbering #469

merged 8 commits into from
Feb 6, 2024

Conversation

matthiasdiener
Copy link
Collaborator

@matthiasdiener matthiasdiener commented Nov 9, 2023 •
edited
Loading

Alternative to #462/#467

cc @MTCam

Please squash

@matthiasdiener matthiasdiener self-assigned this Nov 9, 2023
@matthiasdiener matthiasdiener mentioned this pull request Nov 9, 2023
Closed
@matthiasdiener matthiasdiener mentioned this pull request Nov 28, 2023
Merged
matthiasdiener
matthiasdiener commented Feb 5, 2024
View reviewed changes
test/test_distributed.py

# For the next assertion, find_distributed_partition needs to be
# deterministic too (https://github.com/inducer/pytato/pull/465).
# assert next(iter(distp.parts[0].name_to_send_nodes.values()))[0].comm_tag == 4242 # noqa: E501
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When combining this PR with #465, this assertion passes.

Copy link
Owner

Choose a reason for hiding this comment

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

File an issue to remind us to reenable it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in #482

@matthiasdiener
Copy link
Collaborator Author

This is ready for review @inducer. As mentioned in the comment above, when combining this PR with #465, the (simple) determinism test passes.

@matthiasdiener matthiasdiener marked this pull request as ready for review February 5, 2024 20:51
inducer
inducer approved these changes Feb 6, 2024
View reviewed changes
Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM once one comment is added.

pytato/distributed/tags.py Show resolved Hide resolved
pytato/distributed/tags.py Show resolved Hide resolved
test/test_distributed.py

# For the next assertion, find_distributed_partition needs to be
# deterministic too (https://github.com/inducer/pytato/pull/465).
# assert next(iter(distp.parts[0].name_to_send_nodes.values()))[0].comm_tag == 4242 # noqa: E501
Copy link
Owner

Choose a reason for hiding this comment

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

File an issue to remind us to reenable it?

@matthiasdiener matthiasdiener mentioned this pull request Feb 6, 2024
Open
@inducer inducer merged commit c3fe047 into main Feb 6, 2024
10 checks passed
@inducer inducer deleted the deterministic-mpi_tag-v2 branch February 6, 2024 21:52
@inducer
Copy link
Owner

inducer commented Feb 6, 2024

Thx!

nkoskelo pushed a commit to nkoskelo/pytato that referenced this pull request Mar 5, 2024
@matthiasdiener @nkoskelo
number_distributed_tags: non-set, non-sorted numbering (inducer#469)
5415f64 
* number_distributed_tags: non-set, non-sorted numbering

* make the test a bit more difficult

* improve test

* add comments based on review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inducer inducer inducer approved these changes

Assignees

@matthiasdiener matthiasdiener

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matthiasdiener @inducer