ENH - Gets SciKeras script working #394

lazarust · 2023-10-10T01:00:36Z

WIP still need to fix one test

Reference Issues/PRs

Fixes #388

What does this implement/fix? Explain your changes.

This fixes a recursion error that was happening when dumping a scikeras model.

Any other comments?

WIP still need to fix one test

lazarust · 2023-10-13T13:08:32Z

It looks like all the pytest tests are failing with You have exceeded our daily quotas for action: createRepo. We invite you to retry later.

BenjaminBossan · 2023-10-13T13:24:38Z

Should be addressed by #398

BenjaminBossan · 2023-10-26T15:40:22Z

@lazarust Could you please solve the merge conflict? Regarding the uncovered new line: Would that be solved by adding tests for scikeras?

lazarust · 2023-10-29T00:06:34Z

@BenjaminBossan I've fixed the merge conflict.

Yeah, I believe it would be but I was unsure if we wanted to have tests that included another library like that. Should I add one?

BenjaminBossan · 2023-10-31T09:46:02Z

Yeah, I believe it would be but I was unsure if we wanted to have tests that included another library like that. Should I add one?

Yes, it would be good, since that was the initial reason for the change. We have external library tests, see here:

https://github.com/skops-dev/skops/blob/main/skops/io/tests/test_external.py

For scikeras, it won't be possible to add a comprehensive coverage of all possible models, so a simple example should be enough. If it's possible to add a unit test independently of scikeras that explicitly tests weakrefs, that would also be good, not sure how easy it is to do.

Regarding the failing tests, at first glance, it appears to be a change in the model repr in the latest sklearn version, so not related to the changes here.

lazarust · 2023-11-14T01:36:52Z

@BenjaminBossan Sorry this took me so long to get back to. I've added a test to hit that line.

BenjaminBossan

Thanks so much for the added scikeras tests. I just have a small request to improve them a little bit.

BenjaminBossan · 2023-11-17T14:10:32Z

skops/io/tests/test_external.py

+
+        pipeline = Pipeline([("classifier", clf)])
+
+        dump(clf, "keras-test.skops")


Instead of just dumping, could we please do a cycle of dumps and loads, similar to the other tests?

Done! I now have it dumping the model, loading it back in and comparing the results.

Can we get rid of dump completely in favor of dumps? That way, we also don't need to care about cleaning up any files created during the test.

Fixed! Sorry about that. I didn't realize the difference between dumps and dump 🤯

BenjaminBossan · 2023-11-17T14:13:44Z

@adrinjalali The tests for sklearn nightly are failing because the model repr was changed (not sure why). Normally, we could fix that by having the test check the sklearn verison, but this is a doctest. Any idea how it can be fixed, short of skipping it whole?

BenjaminBossan · 2023-11-22T10:17:55Z

skops/io/tests/test_external.py

+
+        X, y = make_classification(1000, 20, n_informative=10, random_state=0)
+        clf.fit(X, y)
+        dumped = dumps(clf, "keras-test.skops")


Suggested change

dumped = dumps(clf, "keras-test.skops")

dumped = dumps(clf)

2nd argument to dumps is the compression level. Honestly, I'm surprised that this didn't raise an error.

BenjaminBossan · 2023-11-24T13:27:13Z

Ugh, the list of trusted modules is giant now :D I guess it's related to the tensorflow change. Could you please explain why that was necessary? Also, we now get this error on CI:

E AttributeError: module 'scikeras' has no attribute 'wrappers'

lazarust · 2023-11-24T15:39:42Z

@BenjaminBossan Yeah, sorry I realized I had marked the test method as a @pytest.fixture 🤦🏽, so the test wasn't ever running (which is why it wasn't erroring when switching from dump to dumps). I'm hoping to get the test fixed and cleanup that list of trusted modules today.

lazarust · 2023-11-25T01:23:31Z

@BenjaminBossan After staring at this all day, I could use some help lol. For some reason, there's some infinite recursion happening when constructing the tree that I can't figure out why.

Initially, I thought it was due to the CachedNodes in the tree, but the construct method isn't getting hit for those. I've gone through the tree and didn't see anything outrageous so if you could take a look that'd be great!

adrinjalali · 2023-11-25T11:11:03Z

I'll have to check when I'm back. Still off till end of November. But I remember CacheNode caused some issues when I was working on it, partly due to small integers and other common objects having the same id in python.

information for CachedNodes I'm wodering after looking at the types of a lot of the CachedNodes if there's something weird happening with `None`

lazarust · 2024-05-22T13:41:39Z

@adrinjalali I created an issue in tensorflow to discuss this and make sure I wasn't missing anything tensorflow/tensorflow#68194.

It seems like the best way forward is to ignore the warnings for now until Tensorflow supports protobuf 5.0+. Does that sound good to you? Ignoring warnings doesn't sound like the best thing I'm just unsure of a better way forward.

adrinjalali · 2024-05-23T17:10:58Z

Ignoring warnings in cases where we know why they're happening and with a comment as when the ignore statement should be removed is a normal practice indeed. Thanks for the follow up work on the TF side.

lazarust · 2024-05-29T01:05:10Z

@adrinjalali This should be ready for you to look at again. The failing tests seem to just be a blip with codecov

adrinjalali

A few thoughts looking at this in more details (other than the inline comments)

we are delegating the save / load to tensorflow? I'd like to see a test showing the user the right error when they try to load such a model without explicitly trusting including modules.
we don't really include the inner modules in our json tree here, which means we have no idea what's inside that keras model, which means we're prone to massive exploits, this seems it beats the purpose of this format.

@lazarust you've done great work so far, let me know if you need me to have a more detailed look. I haven't personally tried to solve this project for TF, but I could have a look.

skops/io/_scikeras.py

adrinjalali · 2024-06-03T15:29:23Z

skops/io/_scikeras.py

+
+    with tempfile.TemporaryDirectory() as temp_dir:
+        file_name = os.path.join(temp_dir, "model.keras")
+        obj.model.save(file_name)


so we only save the model attribute? This sounds odd.

From my understanding of https://keras.io/guides/serialization_and_saving/, it seems that Keras is compressing all the pieces of the models into the .keras file. Should I change the name of the file to make it more clear?

lazarust · 2024-07-02T01:30:18Z

@adrinjalali I think I've addressed most of your comments, and apologize it took me a bit to get back to this.

For

we don't really include the inner modules in our json tree here, which means we have no idea what's inside that keras model, which means we're prone to massive exploits, this seems it beats the purpose of this format.

I'm a little confused by what you mean by inner modules... could you elaborate on what you mean?

Sorry this PR has been taking so long to get ironed out!

adrinjalali · 2024-08-08T12:50:00Z

@lazarust I went down the rabbit hole of reading the persistence code from keras, and I think it's easier if I push to this PR some changes. So I'll update this one, and you can review the work if that's okay.

lazarust · 2024-08-21T14:09:35Z

@adrinjalali Sounds good to me, let me know if there's any thing I can do to help!

…rking

adrinjalali · 2024-08-28T09:29:59Z

@lazarust , this is what I had in mind.

However, this has a major issue:

The user can use keras.src.saving.saving_lib.save_model with weights_format="npz", or manually create a zip file with npz weights. Then The issue is we have this in tensorflow:

https://github.com/keras-team/keras/blob/d4a51168bfedf69a9aae7ddff289277972dfd85d/keras/src/saving/saving_lib.py#L1027

Which allows loading pickles through numpy. So we can only merge / support this, if we have a way to make sure there are no pickle objects in that zip file.

lazarust · 2024-09-22T15:31:41Z

@adrinjalali Ah the way you used tf directly makes sense to me.

As for the pickle file issue, I don't think there's a good way of doing this... Do you know if the way TF loads the files only looks for .npz files or does it try and load any file in the zip through numpy?

adrinjalali · 2024-09-23T12:17:03Z

I don't think it matters which files TF finds to load, as long as it allows loading pickle files. What we can investigate to move this further, is if we can reliably monkey-patch and disable the pickle machinery while loading a skops file, so that in case there's a pickle load happening somewhere, we make sure we fail.

lazarust · 2024-09-23T20:58:45Z

Yeah I agree, I guess I just thought if TF only tries to load .npz files, maybe skops could just ignore all .npz.

Is monkey-patching TF something we'd want to support long term? If TF changed how model saving/loading worked in a future version we'd have to make updates anyways...

adrinjalali · 2024-09-23T21:14:31Z

It would be monkey patching pickle, not TF.

lazarust · 2024-09-23T21:34:44Z

Ah, yeah I don't think that would be too bad... we'd just need to monkey-patch the load functionality

lazarust · 2024-10-31T01:31:30Z

@adrinjalali Sorry I still haven't gotten to this. Have you made any progress on patching pickle?

adrinjalali · 2024-10-31T10:58:24Z

No I need to get to it soon

lazarust added 3 commits October 9, 2023 19:58

Gets SciKeras script working

925c960

WIP still need to fix one test

Fixes test_metainfo

3500592

Updates changes.rst

492a1ca

lazarust marked this pull request as ready for review October 11, 2023 00:08

Merge branch 'main' into enh-get-scikeras-working

4c91a07

lazarust changed the title ~~Gets SciKeras script working~~ ENH - Gets SciKeras script working Oct 13, 2023

Merge branch 'main' into enh-get-scikeras-working

101b90c

Merge branch 'main' into enh-get-scikeras-working

08f5ba7

lazarust added 2 commits October 28, 2023 19:04

Merge branch 'main' into enh-get-scikeras-working

0491a7b

Update changes.rst

f1b93fe

Adds test

bbe6b34

BenjaminBossan requested changes Nov 17, 2023

View reviewed changes

lazarust added 2 commits November 17, 2023 19:28

Loads dumped model in test and checks output

3b274b4

Refactor test_external.py to use dumps instead of dump

e7ab34e

BenjaminBossan reviewed Nov 22, 2023

View reviewed changes

Add TensorFlow as a dependent package

a1a92cc

lazarust added 2 commits November 24, 2023 12:57

WIP Still running into a recursion error

7b0f21e

Refactor imports and update test method

3397b17

WIP Fix get_state function to include module and class

49a16f0

information for CachedNodes I'm wodering after looking at the types of a lot of the CachedNodes if there's something weird happening with `None`

lazarust added 2 commits May 25, 2024 15:10

Ignores deprecation warning from protobuf

3739f1f

Fixes deprecation warning from matplotlib

ce11bf0

lazarust force-pushed the enh-get-scikeras-working branch from d43cdd7 to ce11bf0 Compare May 25, 2024 20:38

lazarust force-pushed the enh-get-scikeras-working branch from 9ff414a to ce11bf0 Compare June 3, 2024 00:42

adrinjalali reviewed Jun 3, 2024

View reviewed changes

lazarust and others added 5 commits June 9, 2024 14:55

Merge branch 'main' into enh-get-scikeras-working

4c47aaf

Merge branch 'main' into enh-get-scikeras-working

12e2108

Fixes making scikears a hard dependency

d677476

Adds test for error on untrusted types

6d10f71

Cleans up unneeded ()

e307560

lazarust added 2 commits July 13, 2024 20:39

Merge branch 'main' into enh-get-scikeras-working

bb82961

Merge branch 'main' into enh-get-scikeras-working

76341fd

adrinjalali added 4 commits August 28, 2024 11:20

use TF directly

fa6b208

Merge remote-tracking branch 'upstream/main' into enh-get-scikeras-wo…

83891ed

…rking

move changelog

e9b2dd0

add missing file

2d92168

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH - Gets SciKeras script working #394

ENH - Gets SciKeras script working #394

lazarust commented Oct 10, 2023

lazarust commented Oct 13, 2023

BenjaminBossan commented Oct 13, 2023

BenjaminBossan commented Oct 26, 2023

lazarust commented Oct 29, 2023

BenjaminBossan commented Oct 31, 2023

lazarust commented Nov 14, 2023

BenjaminBossan left a comment

BenjaminBossan Nov 17, 2023

lazarust Nov 18, 2023

BenjaminBossan Nov 20, 2023

lazarust Nov 22, 2023

BenjaminBossan commented Nov 17, 2023

BenjaminBossan Nov 22, 2023

BenjaminBossan commented Nov 24, 2023

lazarust commented Nov 24, 2023

lazarust commented Nov 25, 2023

adrinjalali commented Nov 25, 2023

lazarust commented May 22, 2024

adrinjalali commented May 23, 2024

lazarust commented May 29, 2024

adrinjalali left a comment

adrinjalali Jun 3, 2024

lazarust Jul 2, 2024

lazarust commented Jul 2, 2024

adrinjalali commented Aug 8, 2024

lazarust commented Aug 21, 2024

adrinjalali commented Aug 28, 2024

lazarust commented Sep 22, 2024

adrinjalali commented Sep 23, 2024

lazarust commented Sep 23, 2024

adrinjalali commented Sep 23, 2024

lazarust commented Sep 23, 2024

lazarust commented Oct 31, 2024 •

edited

Loading

adrinjalali commented Oct 31, 2024


		pipeline = Pipeline([("classifier", clf)])

		dump(clf, "keras-test.skops")

ENH - Gets SciKeras script working #394

Are you sure you want to change the base?

ENH - Gets SciKeras script working #394

Conversation

lazarust commented Oct 10, 2023

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

lazarust commented Oct 13, 2023

BenjaminBossan commented Oct 13, 2023

BenjaminBossan commented Oct 26, 2023

lazarust commented Oct 29, 2023

BenjaminBossan commented Oct 31, 2023

lazarust commented Nov 14, 2023

BenjaminBossan left a comment

Choose a reason for hiding this comment

BenjaminBossan Nov 17, 2023

Choose a reason for hiding this comment

lazarust Nov 18, 2023

Choose a reason for hiding this comment

BenjaminBossan Nov 20, 2023

Choose a reason for hiding this comment

lazarust Nov 22, 2023

Choose a reason for hiding this comment

BenjaminBossan commented Nov 17, 2023

BenjaminBossan Nov 22, 2023

Choose a reason for hiding this comment

BenjaminBossan commented Nov 24, 2023

lazarust commented Nov 24, 2023

lazarust commented Nov 25, 2023

adrinjalali commented Nov 25, 2023

lazarust commented May 22, 2024

adrinjalali commented May 23, 2024

lazarust commented May 29, 2024

adrinjalali left a comment

Choose a reason for hiding this comment

adrinjalali Jun 3, 2024

Choose a reason for hiding this comment

lazarust Jul 2, 2024

Choose a reason for hiding this comment

lazarust commented Jul 2, 2024

adrinjalali commented Aug 8, 2024

lazarust commented Aug 21, 2024

adrinjalali commented Aug 28, 2024

lazarust commented Sep 22, 2024

adrinjalali commented Sep 23, 2024

lazarust commented Sep 23, 2024

adrinjalali commented Sep 23, 2024

lazarust commented Sep 23, 2024

lazarust commented Oct 31, 2024 • edited Loading

adrinjalali commented Oct 31, 2024

lazarust commented Oct 31, 2024 •

edited

Loading