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.

Should be addressed by #398

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

@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?

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.

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

@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?

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'

@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.

@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!

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.

@adrinjalali I got it working for older versions of Python other than Windows on 3.8 (I think the Ubuntu 3.9 failure was just a codecov issue). Could you help me figure out how to get it working? Downgrading tensorflow to 2.11 just breaks more versions.

One of the main reasons we support 3.8 was colab, which now supports 3.10. I think we can drop 3.8 alltogether.

Would you be kind enough to submit a PR to remove 3.8 support?

@adrinjalali I created this PR to remove 3.8 support #418, but I'm running into an issue where pip install scikit-learn-intelex isn't finding any valid versions only on MacOS (this is happening both in the GH action and locally for me). I'm still looking into a fix

MacOS issue could be this: actions/setup-python#850

Intelex is very brittle, and I don't think it's used much if at all. I'd be in favor of removing it. It was only to handle the inference side and HF inference is not that well maintained anyway.

@BenjaminBossan any objections to remove it?

Works for me.

@lazarust you've been very kind here. Would you remove sklearn-intelex in a separate PR too?

@adrinjalali Done! I removed sklearn-intelex in #420

@lazarust let me know when the CI is green and ready for review here again.

@adrinjalali Sounds good, it may take me a couple of days to get to it. It looks like there's something wrong in tensorflow with how it's using google.protobuf

The errors seem like a bunch of deprecation warnings due to some updates in dependency packages maybe?

@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.

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.

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