-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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.
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
fix: Fixing pytest discovery issues #28158
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Compliance Checks Passed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Vismay-dev, thanks for creating the PR, just requested a couple of changes!
ivy_tests/test_ivy/test_frontends/test_jax/test_numpy/test_indexing.py
Outdated
Show resolved
Hide resolved
@vedpatwardhan i’ve made the changes 🚀 |
@@ -1,7 +1,7 @@ | |||
import ivy | |||
from ivy.func_wrapper import with_unsupported_dtypes | |||
from ivy.functional.frontends.jax.array import Array | |||
import ivy.functional.frontends.jax.numpy as jnp_frontend | |||
import ivy.functional.frontends.jax.numpy as jnp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we have to change the name of the import at either of the 2 places, I'd much rather do that in the test file instead. Sorry for the change of mind, could you please change it so that we do the jnp
import in the test file and jnp_frontend
import in the frontend file? Thanks @Vismay-dev 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vedpatwardhan it's the front end file import that's causing the error rather than the test file. Not the import itself but the use of this namespace jnp_frontend
in the function setdiff1d".
This line https://github.com/unifyai/ivy/blob/d382882aafb0e19cccc8145cfcaa6cc4f53a77c4/ivy/functional/ivy/data_type.py#L192 checks for the keyword "frontend" in the base names of all the functions invoked in the frontend function setdiff1d
. I'm not sure if this is an intentional design caveat of the current use of an AST, or just an unprecedented edge case.
Changing the import in the test file, as you originally pointed out, is not related to the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's probably change the import in the frontend file to something like from ivy.functional.frontends.jax import where, unique
similar to the other imports in the jax
frontend if that fixes the issue 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vedpatwardhan that's causing a circular import error. the reason there's no circular import error for the other imports in the jax
frontend is because those imported functions (for eg. promote_types_of_jax_inputs
) have been defined in __init__.py
for the frontends/jax/numpy
frontend directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed a change to data_type.py
, does test collection now work with jnp_frontend
in the frontend file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the test collection works with jnp_frontend
now 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome! We can get the PR merged after the changes to the demos
submodule are reverted. Feel free to request a review once you're done with that, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vedpatwardhan all done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! Feel free to merge the PR @NripeshN, thanks @Vismay-dev 😄
Hey @Vismay-dev @vedpatwardhan Find the entire log below
|
This reverts commit 3636f44.
This error is related to array_api_testing suite. I've fixed the issue with numpy bool attribute on my local machine. How do I go about pushing changes to array_api_testing suite? As per the deep dive, this cannot be done without a submodule update. Pytest discovery works for Also, I still face this error, though I presume this is because of my setup.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @NripeshN, as @Vismay-dev mentioned, setting
"python.testing.pytestArgs": [
"ivy_tests/test_ivy"
],
in the settings.json
should help ignore the array api tests 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!!
PR Description
Fixing pytest discovery issues
Specific edge cases addressed:
ivy.functional.frontends.jax.numpy.indexing.CClass
is a class object invoked as a function call, parsed in ast extraction.jnp_frontend
namespace causing string manipulation error in retrieving dtypes/device for nested functions.mindspore
version.Related Issue
Closes #28138
Checklist
Socials