Sklearn autologging: Fix behavior when a child and its parent are both patched

[dbczumar]

What changes are proposed in this pull request?

Fixes #3574 by addressing the following two issues:

1. In gorilla 0.3.0, applying a patch that stores a hit to a parent class (e.g., CountVectorizer) forces get_original_attribute of its children to refer to the parent class attribute, even if we patch the child later on. I.e.

This ordering is okay

gorilla.Patch(<TfidfVectorizer>)
gorilla.Patch(<CountVectorizer>)

but this ordering is not

gorilla.Patch(<CountVectorizer>)
gorilla.Patch(<TfidfVectorizer>)

This appears to be caused by the following line that prevents the original attribute from being overwritten once it's set: https://github.com/christophercrouzet/gorilla/blob/0c895b34311b6546bde35854e24862c001279add/gorilla.py#L280. As a result, setting the attribute on the parent, which sets the original attribute on the child if the child does not already define it, means that the child is always stuck with the parent's original attribute. This seems to be fixed in the master branch of gorilla but has not been released. Either we can pull in the copy from master or pull in a copy from 0.3.0 that removes this problematic hasattr check.

2. In fit_mlflow and fit_predict, we perform get_original_attribute(self, func_name). In the case where we're in a super class function (e.g., CountVectorizer.fit_transform()), self still refers to the subclass (e.g., TfidfVectorizer). As a result, calling get_original_attribute in the patched super class method fetches invokes the patched subclass method, which creates the infinite recursion problem observed in the issue description. I've filed Sklearn autologging: Fix behavior when a child and its parent are both patched #3582 to address this problem by fetching the original function attribute by reference to a class rather than by reference to self. This PR still needs docs & tests, and it needs to incorporate a fix for (1) before it's mergeable.

How is this patch tested?

Unit tests

Release Notes

Fixes an infinite recursion bug in scikit-learn autologging caused by invoking a fit() method on a model class that includes a call to super.fit() as part of its implementation (see #3574).

Is this a user-facing change?

• No. You can skip the rest of this section.
• Yes. Give a description of this change to be included in the release notes for MLflow users.

What component(s), interfaces, languages, and integrations does this PR affect?

Components

• area/artifacts: Artifact stores and artifact logging
• area/build: Build and test infrastructure for MLflow
• area/docs: MLflow documentation pages
• area/examples: Example code
• area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
• area/models: MLmodel format, model serialization/deserialization, flavors
• area/projects: MLproject format, project running backends
• area/scoring: Local serving, model deployment tools, spark UDFs
• area/server-infra: MLflow server, JavaScript dev server
• area/tracking: Tracking Service, tracking client APIs, autologging

Interface

• area/uiux: Front-end, user experience, JavaScript, plotting
• area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
• area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
• area/windows: Windows support

Language

• language/r: R APIs and clients
• language/java: Java APIs and clients
• language/new: Proposals for new client languages

Integrations

• integrations/azure: Azure and Azure ML integrations
• integrations/sagemaker: SageMaker integrations
• integrations/databricks: Databricks integrations

How should the PR be classified in the release notes? Choose one:

• rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
• rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
• rn/feature - A new user-facing feature worth mentioning in the release notes
• rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
• rn/documentation - A user-facing documentation change worth mentioning in the release notes

[harupy]

@dbczumar Is this a fix for #3574 ?

[dbczumar]

@harupy These are the critical portions of the change to address problem (1) from #3574 (comment).

We now retain a reference to the previous patch that was applied to a class or one of its super classes, and, if the patch was applied to a super class, we overwrite it with the patch that's being applied to this class directly. See https://github.com/christophercrouzet/gorilla/blob/0045a7f4b5c46bda208dbce9e628f97bc9e551e0/gorilla.py#L295-L300 for the original implementation.

[dbczumar]

This (together line 914) is the critical change to address problem (2) from #3574 (comment).

Assume that the old line was being invoked from a patched fit call on a class called Parent and that self refers to an instance of class Child which inherits from Parent. Calling get_original_attribute() on self would yield the Child.fit() function, rather than Parent.fit(), leading to recursive behavior.

Now, instead of fetching the attribute from self, we fetch it from clazz (which would refer to Parent in the above example).

[harupy]

LGTM 👍