`ENH` trusting np.ufuncs and np.dtypes by omar-araboghli · Pull Request #336 · skops-dev/skops

omar-araboghli · 2023-04-01T11:57:30Z

Reference Issues/PRs

closes #224

What does this implement/fix? Explain your changes.

Trusting numpy.ufuncs and numpy.dtypes by default.

Any other comments?

I compared the ufuncs list obtained in this PR with the available ufuncs. They are almost the same except the aliases. Take true_divide as an example that is an alias for divide and unfortunately cannot be found with the dir(...) and isinstance(...) approach. Is there a way to find numpy aliases ?
test_can_persist_fitted didn't run successfully until I added NUMPY_DTYPE_TYPE_NAMES to the default trusted list of NdArrayNode. I am wondering why isn't DTypeNode the correct place ? Is there any other Nodes I am missing ?

…ypes has the same type.

adrinjalali

Thanks @omar-araboghli

BenjaminBossan

Take true_divide as an example that is an alias for divide and unfortunately cannot be found with the dir(...) and isinstance(...) approach. Is there a way to find numpy aliases

Not that I know of, but it's better to trust too little than too much, so it's fine for now.

test_can_persist_fitted didn't run successfully until I added NUMPY_DTYPE_TYPE_NAMES to the default trusted list of NdArrayNode

This might be a bug, let me investigate before we merge this.

BenjaminBossan · 2023-04-03T09:13:06Z

test_can_persist_fitted didn't run successfully until I added NUMPY_DTYPE_TYPE_NAMES to the default trusted list of NdArrayNode

This might be a bug, let me investigate before we merge this.

Okay, I believe this is not a bug. The reason is that we use NdArrayNode not only for numpy arrays, but also for numpy generics. If e.g. a numpy float is saved, it will use NdArrayNode, which is why the numpy dtypes have to be trusted there. This is indeed a bit confusing, maybe another name for the class would have been better.

Another source of confusion (at least for me) is what is meant by dtype. Here we mean e.g. a numpy.float64. However, there is also the "dtype" type, e.g. numpy.dtype[float64], which is persisted using the DtypeNode.

At the end of the day, I think everything is working here as expected, but the naming can cause confusion. What's your opinion @adrinjalali?

adrinjalali · 2023-04-03T09:53:20Z

Changing the names here wouldn't be backward compatible, and will probably take some time, and I also don't have very good ideas for their names. So I think we can leave that for a future iteration.

BenjaminBossan · 2023-04-03T10:01:56Z

Changing the names here wouldn't be backward compatible, and will probably take some time, and I also don't have very good ideas for their names. So I think we can leave that for a future iteration.

True, we should probably add a comment though.

@omar-araboghli I think the docs also need to be updated here: https://skops.readthedocs.io/en/stable/persistence.html#usage

omar-araboghli · 2023-04-11T10:14:42Z

@adrinjalali and @BenjaminBossan thanks for the review and clarification! Please let me know in case something still has to be changed.

I defined the function get_public_type_names that does the thing we need to do twice for now and used walrus operator as suggested, also multiple times. Does its name make sense ?

adrinjalali

Otherwise it's looking great I think.

adrinjalali · 2023-04-13T09:03:43Z

    return [get_type_name(t) if not isinstance(t, str) else t for t in types]
+
+
+def get_public_type_names(module: ModuleType, _type: Type) -> list[str]:


maybe rename _type to oftype?

adrinjalali · 2023-04-13T09:04:37Z

+        {
+            type_name
+            for attr in dir(module)
+            if (isinstance(obj := getattr(module, attr), _type))


isinstance or issubclass?

changing it to

Suggested change

if (isinstance(obj := getattr(module, attr), _type))

if (issubclass((obj := getattr(module, attr)).__class__, _type))

gets the same results for both numpy and scipy. Since the ufuncs are classes and not objects (instances), issubclass makes more sense to use here. Thanks for the suggestion!

…type

…4-numpy-trusted-types

BenjaminBossan

Looks quite good, thx. Only two minor issues for me, otherwise the PR can be merged.

BenjaminBossan · 2023-04-13T13:57:46Z



 class NdArrayNode(Node):
+    # TODO: NdArrayNode and DtypeNode names lead to confusion, see PR-336


Instead of sending users on a chase, let's just clear the confusion right away: That this is not only responsible for np arrays, but also for np generics.

BenjaminBossan · 2023-04-13T13:59:11Z

+            if (issubclass((obj := getattr(module, attr)).__class__, oftype))
+            and ((type_name := get_type_name(obj)).startswith(module_name))


For both lines, are the outer parens necessary? It seems to me that they could be removed.

wasn't easy to use walrus operator in such "complex" statement 😄 Thanks for the catch - removed them!

BenjaminBossan · 2023-04-20T10:21:43Z

@omar-araboghli is this ready for review?

omar-araboghli · 2023-04-20T10:27:12Z

@omar-araboghli is this ready for review?

@BenjaminBossan Indeed! Sorry for not pingging you.

BenjaminBossan

Thanks for implementing the suggestions, this looks good now. You'd need to merge the current main and fix the potential merge conflict in changes.rst, then this is good to go.

It would have been nice if NUMPY_DTYPE_TYPE_NAMES could also use get_public_type_names but it wouldn't work as is. If you happen to come up with a clever solution to that works for both, it would be good to make that change, but it's not a blocker.

BenjaminBossan · 2023-04-26T09:23:59Z

I just noticed that test_external.py can also be cleaned up, as it contains a couple of numpy dtypes that no longer need to be trusted explicitly.

omar-araboghli · 2023-05-14T14:14:00Z

@BenjaminBossan just resolved the conflict and cleaned test_externals.py as well as test_visualize.py up. I unfortunately couldn't come up with a generic solution to have NUMPY_DTYPE_TYPE_NAMES use get_public_type_names since numpy doesn't have a superclass covering them. Getting them from np.sctypes seems to be the simplest way to go with. Any hint on that or shall we keep it as it as for now ?

BenjaminBossan

Great work cleaning up the remaining instances, this looks very good now. There is a small typo in one of the docstrings, besides that this can be merged.

I unfortunately couldn't come up with a generic solution

Then it's fine, it would have been nice but it's not a requirement.

Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>

omar-araboghli · 2023-05-15T12:23:48Z

@BenjaminBossan Thanks for the review and great catch! The typo is now resolved, thanks to @adrinjalali :)

BenjaminBossan

Great work, thx

omar-araboghli added 2 commits April 1, 2023 13:47

skops-dev#224: trusting np.ufuncs and np.dtypes

89561a0

skops-dev#224: updating changes.rst

22b0043

omar-araboghli changed the title ~~#224: trusting np.ufuncs and np.dtypes~~ ENH trusting np.ufuncs and np.dtypes Apr 1, 2023

omar-araboghli added 2 commits April 1, 2023 14:15

skops-dev#224: fixing failing tests.

3fc5116

skops-dev#224: ignoring mypy errors. Should be safe as long as np.sct…

3ad5f78

…ypes has the same type.

adrinjalali reviewed Apr 3, 2023

View reviewed changes

Comment thread skops/io/tests/test_audit.py Outdated

Comment thread skops/io/tests/test_audit.py Outdated

BenjaminBossan requested changes Apr 3, 2023

View reviewed changes

Comment thread skops/io/_trusted_types.py Outdated

Comment thread skops/io/_trusted_types.py Outdated

Comment thread skops/io/tests/test_audit.py Outdated

omar-araboghli added 3 commits April 11, 2023 10:47

Merge branch 'main' into 224-numpy-trusted-types

06f4bb9

skops-dev#224: resolving review remarks.

71818db

Merge branch 'main' into 224-numpy-trusted-types

dae61fb

omar-araboghli requested a review from BenjaminBossan April 11, 2023 10:11

adrinjalali reviewed Apr 13, 2023

View reviewed changes

omar-araboghli added 2 commits April 13, 2023 11:49

skops-dev#224: replacing isinstance with issubclass and _type with of…

98661be

…type

Merge remote-tracking branch 'origin/224-numpy-trusted-types' into 22…

abc5ceb

…4-numpy-trusted-types

adrinjalali approved these changes Apr 13, 2023

View reviewed changes

BenjaminBossan requested changes Apr 13, 2023

View reviewed changes

skops-dev#224: clarifying the confusion and removing extra parentheses.

f9e1cba

BenjaminBossan approved these changes Apr 21, 2023

View reviewed changes

omar-araboghli added 2 commits May 14, 2023 15:18

skops-dev#224: updating branch and fixing conflicts

bc2099c

skops-dev#224: cleaning up tests with explicit numpy.dtypes

e0e951f

BenjaminBossan requested changes May 15, 2023

View reviewed changes

Comment thread skops/io/_utils.py Outdated

Update skops/io/_utils.py

fb69a45

Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>

adrinjalali enabled auto-merge (squash) May 15, 2023 10:18

omar-araboghli requested a review from BenjaminBossan May 15, 2023 12:22

BenjaminBossan approved these changes May 15, 2023

View reviewed changes

adrinjalali merged commit c68141e into skops-dev:main May 15, 2023

		return [get_type_name(t) if not isinstance(t, str) else t for t in types]


		def get_public_type_names(module: ModuleType, _type: Type) -> list[str]:

	if (isinstance(obj := getattr(module, attr), _type))
	if (issubclass((obj := getattr(module, attr)).__class__, _type))



		class NdArrayNode(Node):
		# TODO: NdArrayNode and DtypeNode names lead to confusion, see PR-336

		if (issubclass((obj := getattr(module, attr)).__class__, oftype))
		and ((type_name := get_type_name(obj)).startswith(module_name))

Conversation

omar-araboghli commented Apr 1, 2023

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BenjaminBossan commented Apr 3, 2023

Uh oh!

adrinjalali commented Apr 3, 2023

Uh oh!

BenjaminBossan commented Apr 3, 2023

Uh oh!

omar-araboghli commented Apr 11, 2023

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

adrinjalali Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

adrinjalali Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

omar-araboghli Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

omar-araboghli Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan commented Apr 20, 2023

Uh oh!

omar-araboghli commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan commented Apr 26, 2023

Uh oh!

omar-araboghli commented May 14, 2023

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

omar-araboghli commented May 15, 2023

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

omar-araboghli commented Apr 20, 2023 •

edited

Loading