-
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
Merged
Merged
Changes from 5 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
0a5a609
Fixing pytest discovery issues
vismaysur d5b21e5
Made helper fn get_tf_keras_activations private
vismaysur b7b4afc
Made helper fn get_tf_keras_activations private
vismaysur fff49b1
Made helper fn get_tf_keras_activations private
vismaysur 9720072
changed jnp import namespace
vismaysur bea8b9e
Merge branch 'unifyai:main' into fix_pytest_discovery
vismaysur ff5ae3d
Update data_type.py
vedpatwardhan cc4850f
updated jnp_frontend import
vismaysur e0d235b
Revert "updated jnp_frontend import"
vismaysur 2b7c88a
update jnp_frontend import
vismaysur 5a38ecf
Revert "update jnp_frontend import"
vismaysur 3636f44
update jnp_frontend import
vismaysur b2858e1
Merge remote-tracking branch 'upstream/main' into pr/28158
NripeshN eda72e9
support numpy attr bool_
vismaysur 2841ae7
Revert "update jnp_frontend import"
vismaysur 20dfaff
jnp_frontend import
vismaysur 0370069
update demos submodule
vismaysur 73d961c
jnp_frontend import
vismaysur adc3737
Reverted change to test_array_api
vismaysur e8f7fa0
Merge remote-tracking branch 'upstream/main' into pr/28158
NripeshN File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
"sklearn": "1.3.0", | ||
"xgboost": "1.7.6", | ||
"torchvision": "0.15.2.", | ||
"mindspore": "2.0.0", | ||
} | ||
|
||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 andjnp_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 functionsetdiff1d".
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 thejax
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 thefrontends/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 withjnp_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!