MNT add python=3.11 support #246
Merged
Conversation
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 raises this It seems legit, but not sure why my local environment doesn't reproduce it. Any ideas, would you have an idea @jeremiedbb @jjerphan? This fails on our nightly build. |
|
Hi @adrinjalali, Can you report your packages versions using |
|
I know @jjerphan , I have a py311 local env. But I'm not getting the warning locally. Locally, I have: And on the CI, we have: |
|
I've opened scikit-learn/scikit-learn#25157 accordingly. |
|
Probably you already have those datasets cached on your disk, hence not running the deprecated functions and thus not having the deprecation warning which itself being promoted as an error. |
BenjaminBossan
added a commit
to BenjaminBossan/skops
that referenced
this pull request
Dec 9, 2022
- Remove Python 3.7 compatibility code (requires skops-dev#246) - Removed empty section in docs - Don't add empty metrics table by default - Remove option to select with a list of str (e.g. card.select(['Foo', 'Bar'])) is no longer possible) - Add option to chain selections using select method (e.g. card.select('Foo').select('Bar'))
|
@adrinjalali This warnings filter worked for me locally:
The key seems to be this:
(docs) This always trips me as well. |
E-Aho
pushed a commit
that referenced
this pull request
Dec 16, 2022
Description The proposed model card implementation would allow to dynamically add sections or overwrite them. This is not a complete implementation but already covers most of the features we already have and then some. On top of these features, it would be possible to add more features like creating a default Card with placeholders, just like the exisint template, or the possibility to delete existing sections or to retrieve the result of a certain section. Implementation The underlying data structure consists of a dict and a Section dataclass. All data is stored in a _data attribute with the type dict[str, Section]. The dataclass hold the section contents, i.e. the section title, the section content, and subsections, which again have the same type. It's thus recursive data structure. Section title and dict key are identical, which is mostly for convenience. With this refactor, there are no separate data containers anymore for eval results, template sections, extra sections, etc. They are all treated the same. IMHO, this greatly simplifies the code overall. The only complex function that's left is the one needed to traverse the tree holding the data, and even that is just 14 LOC. Demo To see how the new class can be used, take a look at the main function. The resulting Card can be seen here: https://huggingface.co/skops-ci/hf_hub_example-fcc0d6fe-d072-4f94-8fdb-6bf3bb917bca * [WIP] Further align new model card design Added a test that shows that the new card produces the same output as the old card (except for a few non-deterministic parts). This includes most of the idiosyncrasies of the old card we might want to change in the future (e.g. inconsistent capitalization, use of empty lines). Some of the more problematic behaviors of the old card class were, however, fixed (e.g. creating an empty metrics table when there are no metrics). The other tests have been reworked to use the new card features to make them more precise. Often, that means that instead of having a very weak test like "assert 'foo' in card.render()", it is now possible to select the exact section and check that it equals the expected output. This work is still unfinished, specifically it still lacks tests for the card repr and for the newly added features. * Make tests pass Some refactoring to clean up things, rework repr, make repr tests pass. * Add tests for new functionalities and docstrings * Adjust tests to work with older sklearn versions * Adjust examples to new card, more docs Also some better type annotations. * Continue fixing tests * Try fixing Windows error by specifying encoding * Adjust doctest: confusion matrix not stored in cwd * Increase test coverage * Try fixing test failure on Windows * Replace old by new Card implementation * Address reviewer comments - Remove noise from docstring example - Add the comma after model repr - Add docstrings to private methods * Add TODO notes for when Python 3.7 is dropped * Add Hub model card template, add template arg Users can now choose to use no template, skops template, hub template, or their own template. Using their own template disables a lot of prefilling (say, putting the model plot in the card) because we wouldn't know where to put it. Users will need to call card.add for the otherwise prefilled sections. * Make _add_single return the Section This can be useful, because otherwise it takes a bit of effort to retrieve the latest section. * Allow tables without rows to be added It's ugly, but there is no technical reason from prohibiting the addition of tables without rows. (Note, columns are still required). This allows us to use TableSection for formatting the metrics, instead of calling tabulate there directly. This is better, since we don't have 2 separate ways of creating metrics. * Error when calling add_metric w/ invalid template * Some amendments required after merging Most notably, dynamic model loading in Cards was broken. * A couple more changes: 1. Don't use Hub template for now 2. Document how to add new default templates in code 3. Nicer error message when using invalid template * Add entry to changes.rst * Fix small bug in generated code for loading * Address reviewer comments - Remove Python 3.7 compatibility code (requires #246) - Removed empty section in docs - Don't add empty metrics table by default - Remove option to select with a list of str (e.g. card.select(['Foo', 'Bar'])) is no longer possible) - Add option to chain selections using select method (e.g. card.select('Foo').select('Bar')) * Adjust getting started code in model card * Adjust docstrings * Allow adding default sections with custom template So far, if users had a custom template (including no template at all), they would lose the possibility to add some default sections: - metrics - model plot - hyperparamters - getting started code Now we expose methods to the users to add those sections with a simple call (no need to manually format the content and use card.add). For this to work, users have to indicate the desired section, since we would otherwise not know where to put the content. During the work on this, I also cleaned up the Card tests, which became messier and messier over time. Related tests are now all contained in a class, which makes it a little bit easier to see if for a certain method, all test cases have been covered.
|
I tinkered a bit and found a way to make the tests pass with Python 3.11 (starting from main branch). The patch is shown below. The most notable change is with regard to allowing diff --git a/skops/io/_general.py b/skops/io/_general.py
index 10ef9a0..9bc2254 100644
--- a/skops/io/_general.py
+++ b/skops/io/_general.py
@@ -401,10 +401,11 @@ class ObjectNode(Node):
return instance
attrs = self.children["attrs"].construct()
- if hasattr(instance, "__setstate__"):
- instance.__setstate__(attrs)
- else:
- instance.__dict__.update(attrs)
+ if attrs is not None:
+ if hasattr(instance, "__setstate__"):
+ instance.__setstate__(attrs)
+ else:
+ instance.__dict__.update(attrs)
return instance
diff --git a/skops/io/_sklearn.py b/skops/io/_sklearn.py
index 9da5392..997cc28 100644
--- a/skops/io/_sklearn.py
+++ b/skops/io/_sklearn.py
@@ -71,6 +71,9 @@ def reduce_get_state(obj: Any, save_context: SaveContext) -> dict[str, Any]:
# reduce includes what's needed for __getstate__ and we don't need to
# call __getstate__ directly.
attrs = reduce[2]
+ elif len(reduce) == 2:
+ # should be a tuple of class, args
+ _, attrs = reduce
elif hasattr(obj, "__getstate__"):
attrs = obj.__getstate__()
elif hasattr(obj, "__dict__"):
@@ -78,7 +81,7 @@ def reduce_get_state(obj: Any, save_context: SaveContext) -> dict[str, Any]:
else:
attrs = {}
- if not isinstance(attrs, dict):
+ if not isinstance(attrs, (dict, tuple)):
raise UnsupportedTypeException(
f"Objects of type {res['__class__']} not supported yet"
)
@@ -119,8 +122,13 @@ class ReduceNode(Node):
if hasattr(instance, "__setstate__"):
instance.__setstate__(attrs)
- else:
+ elif isinstance(attrs, dict):
instance.__dict__.update(attrs)
+ else:
+ # we (probably) got tuple attrs but cannot setstate with them
+ raise UnsupportedTypeException(
+ f"Objects of type {constructor} are not supported yet"
+ )
return instance
diff --git a/skops/io/tests/_utils.py b/skops/io/tests/_utils.py
index ead14b2..bd73670 100644
--- a/skops/io/tests/_utils.py
+++ b/skops/io/tests/_utils.py
@@ -67,10 +67,13 @@ def _assert_tuples_equal(val1, val2):
def _assert_vals_equal(val1, val2):
- if hasattr(val1, "__getstate__"):
+ if type(val1) == type: # e.g. could be np.int64
+ assert val1 is val2
+ elif hasattr(val1, "__getstate__") and (val1.__getstate__() is not None):
# This includes BaseEstimator since they implement __getstate__ and
# that returns the parameters as well.
- #
+ # Since Python 3.11, all objects have a __getstate__ but they return
+ # None by default, in which case this check is not performed.
# Some objects return a tuple of parameters, others a dict.
state1 = val1.__getstate__()
state2 = val2.__getstate__()
|
|
@adrinjalali I'm happy with the PR and would be ready to merge it. However, I'm a bit concerned about the uncovered lines of code. I think we should investigate before proceeding. I checked out this branch and ran it locally with Python 3.10 and 3.11. Interestingly, I could throw out a lot of code from def _construct(self):
args = self.children["args"].construct()
constructor = self.children["constructor"]
instance = constructor(*args)
attrs = self.children["attrs"].construct()
if not attrs:
# nothing more to do
return instance
# ALL THE CODE HERE WAS DELETED
instance.__setstate__(attrs)
return instanceAlso, locally, this line was not covered, not sure why codecov doesn't highlight that (maybe it's covered for Python < 3.10?). |
|
The reason those lines exist is to handle cases where somebody manufactures a |
Okay, then at the very least we should add a test for those manufactured states, right? Then I can merge this and an issue should be created. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #245