Skip to content

Conversation

@hugolatendresse
Copy link
Contributor

Add support for nn.CrossEntropyLoss in exported program translator.

general_reduction.py would fail when the reduction would reduce to a scalar. This PR allows the last block being a scalar.

assert_torch_output_vs_tvm_from_exported_to_cuda(raw_data, torch_module, target, dev)


@tvm.testing.parametrize_targets("cuda")
Copy link
Contributor

@mshr-h mshr-h Apr 21, 2025

Choose a reason for hiding this comment

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

I'm not confident whether we need e2e tests for every ops.
The upside is we may not have to care about pytorch incompatible API update such as #17680 we hit recently.
The downside is that they increase the CI pressure, which results in a slow development speed.
Maybe nightly is a good place for e2e tests?
cc @Hzfengsy @tqchen @yongwww

Copy link
Member

@tqchen tqchen Apr 21, 2025

Choose a reason for hiding this comment

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

I agree we should avoid exccessive e2e tests and infavor of unit tests, we should move the execution based tests to something likely a nightly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I will undo that and also update #17862

Copy link
Contributor Author

@hugolatendresse hugolatendresse Apr 21, 2025

Choose a reason for hiding this comment

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

One reason why I added those tests is that we already had support for torch.sort (with a unit test), but when I tested e2e I realized that we had differerent behavior than pytorch. Maybe it's worth it indeed to add those tests to nightly? If so, what is the way to do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nightly tests are located at tests/python/nightly.

Copy link
Contributor Author

@hugolatendresse hugolatendresse May 4, 2025

Choose a reason for hiding this comment

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

I moved the e2e tests to nightly in #17907 (the commits are also in this PR to avoid future conflicts)

Copy link
Contributor

@mshr-h mshr-h left a comment

Choose a reason for hiding this comment

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

CrossEntropyLoss support looks good to me.

@hugolatendresse
Copy link
Contributor Author

@mshr-h @tqchen @MasterJH5574 I removed the e2e tests.

@hugolatendresse hugolatendresse marked this pull request as draft April 22, 2025 14:32


def test_cross_entropy():
class CrossEntropyModule(Module):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be better to include a test case for functional cross entropy.

Copy link
Contributor Author

@hugolatendresse hugolatendresse May 4, 2025

Choose a reason for hiding this comment

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

@mshr-h I added a e2e test case in the nightly tests (last lines of tests/python/nightly/test_nnapi/test_from_exported_to_cuda.py).
Please let me know if that addresses your concern.

Fix reduction for sort and cross-entropy
@hugolatendresse hugolatendresse marked this pull request as ready for review May 4, 2025 22:39
@yongwww yongwww merged commit d3e3d43 into apache:main May 8, 2025
11 checks passed
ShiboXing pushed a commit to ShiboXing/tvm that referenced this pull request Aug 10, 2025
* tests for add'l modules

* no sort

* cross entropy test passes

* cleanup

* fix expand

* remove new e2e tests

* remove new e2e tests

* convert e2e test to unit test

* unit test

* restore tests

* move

* add new tests

* add new tests from 17862

* whitespace

* print statemetns

* all tests pass

* cleanup - all tests still pass

* cleanup. All nightly tests pass
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.

4 participants