Skip to content

Conversation

@aobolensk
Copy link
Contributor

  • Fix langctx module import in thunder.numpy
  • Add basic tests for thunder.numpy module

- Fix langctx module import in thunder.numpy
- Add basic tests for thunder.numpy module
Copy link
Collaborator

@IvanYashchuk IvanYashchuk left a comment

Choose a reason for hiding this comment

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

There's no harm in merging this PR. However, the untested state of the NumPy language context suggests that it hasn't been one of the most important aspects of Thunder for a while. It may be time to retire it and lighten the codebase.

Copy link
Collaborator

@riccardofelluga riccardofelluga left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! just a few nits and questions

Comment on lines +14 to +16
assert len(t) == 2 # axis 0 length
assert t.size() == 6 # total elements
assert np_size(t) == 6
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you choose to test these?

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 needed to check that any numpy function is actually working correctly under the context manager. If you have any better ideas for testing, please, suggest one, I'm ready to fix



def test_numpy_langctx_registration_and_len_size():
with detached_trace():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice trick here

from collections.abc import Callable

from thunder.core.langctx import langctx, Languages
from thunder.core.langctxs import langctx, Languages
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great catch!

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.

5 participants