Skip to content

COO intersection: allow external hash + hash reuse in sparse_mask#94596

Closed
nikitaved wants to merge 19 commits intogh/nikitaved/19/basefrom
gh/nikitaved/19/head
Closed

COO intersection: allow external hash + hash reuse in sparse_mask#94596
nikitaved wants to merge 19 commits intogh/nikitaved/19/basefrom
gh/nikitaved/19/head

Conversation

@nikitaved
Copy link
Copy Markdown
Collaborator

@nikitaved nikitaved commented Feb 10, 2023

@pytorch-bot
Copy link
Copy Markdown

pytorch-bot bot commented Feb 10, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/94596

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 Failures

As of commit 2c3bfc6:

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

nikitaved added a commit that referenced this pull request Feb 10, 2023
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
nikitaved added a commit that referenced this pull request Feb 13, 2023
Comment on lines +280 to +285
auto hash = source_indices_hash_opt.has_value()
? (*source_indices_hash_opt).contiguous()
: at::empty({probably_coalesced._nnz()}, indices.options().dtype(kLong));
const auto* RESTRICT hash_ptr = source_indices_hash_opt.has_value()
? hash.data_ptr<int64_t>()
: nullptr;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Bug: wrong place to put it.

…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
nikitaved added a commit that referenced this pull request Feb 13, 2023
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
nikitaved added a commit that referenced this pull request Feb 15, 2023
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
…se_mask"

External hash implies more flexibility in the op coverage.
Next up: `index_select`.




[ghstack-poisoned]
@nikitaved nikitaved requested review from amjames, cpuhrsch and pearu March 13, 2023 13:55

namespace native {

using OptTensor = c10::optional<Tensor>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Adding this to the header makes it too widely available.

@cpuhrsch
Copy link
Copy Markdown
Contributor

Reuse makes sense but why do we need to be able to control the hash?

@nikitaved
Copy link
Copy Markdown
Collaborator Author

nikitaved commented Mar 14, 2023

@cpuhrsch , yes, it allows to implement intersections along a specific dimension, and so is broadcasting all with the same kernel. All these cases require different hash functions to be computed.

Copy link
Copy Markdown
Contributor

@cpuhrsch cpuhrsch left a comment

Choose a reason for hiding this comment

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

Can we remove using OptTensor from the headers? Otherwise this is good to go.

…se_mask"

External hash implies more flexibility in the op coverage + performance improvement.




[ghstack-poisoned]
@nikitaved
Copy link
Copy Markdown
Collaborator Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Mar 15, 2023
@pytorchmergebot
Copy link
Copy Markdown
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / win-vs2019-cuda11.7-py3 / test (default, 3, 5, windows.g5.4xlarge.nvidia.gpu)

Details for Dev Infra team Raised by workflow job

@nikitaved
Copy link
Copy Markdown
Collaborator Author

@pytorchbot merge -m "unrelated failure"

@pytorch-bot
Copy link
Copy Markdown

pytorch-bot bot commented Mar 15, 2023

❌ 🤖 pytorchbot command failed:

@pytorchbot: error: unrecognized arguments: -m unrelated failure

usage: @pytorchbot [-h] {merge,revert,rebase,label,drci} ...

Try @pytorchbot --help for more info.

@nikitaved
Copy link
Copy Markdown
Collaborator Author

@pytorchbot merge -f "unrelated failure"

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 23, 2023
…4596)

External hash implies more flexibility in the op coverage + performance improvement.

Pull Request resolved: pytorch/pytorch#94596
Approved by: https://github.com/cpuhrsch
cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 27, 2023
…4596)

External hash implies more flexibility in the op coverage + performance improvement.

Pull Request resolved: pytorch/pytorch#94596
Approved by: https://github.com/cpuhrsch
@facebook-github-bot facebook-github-bot deleted the gh/nikitaved/19/head branch June 8, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged open source release notes: sparse release notes category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants