deps: remove scikit-learn and sqlalchemy as required dependencies #1296

Genesis929 · Jan 16, 2025

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

tswast · Jan 21, 2025

bigframes/ml/metrics/_metrics.py

+    if len(x_pandas) < 2:
+        raise ValueError(
+            f"At least 2 points are needed to compute area under curve, but x.shape = {len(x_pandas)}"
+        )
+
+    if x_pandas.is_monotonic_decreasing:
+        d = -1
+    elif x_pandas.is_monotonic_increasing:
+        d = 1
+    else:
+        raise ValueError(f"x is neither increasing nor decreasing : {x_pandas}.")
+
+    if hasattr(np, "trapezoid"):
+        # new in numpy 2.0
+        return d * np.trapezoid(y_pandas, x_pandas)
+    # np.trapz has been deprecated in 2.0
+    return d * np.trapz(y_pandas, x_pandas)  # type: ignore


If this code is copied from sklearn, we should put it in the third_party/bigframes_vendored directory.

This is a simplified version from sklearn logic, because we need it to work for pandas series only. In this case should we put it in third-party?

Yes. If it originally came from somewhere else, we need to show attribution for the copyright. Even if we modified it.

Moved to third party

tswast · Jan 21, 2025

noxfile.py

+    session.install(
+        "scikit-learn>=1.2.2",
+    )


Do not install optional dependencies in unit_noextras. The intention of such test sessions is to make sure that dependencies that are marked as optional are truly optional. This risks making a forced dependency on this package.

Also, prefer removing the package from other sessions as well.

Updated, now test skipped is sklearn not installed.

For docs and docfx session, it's required to have sklearn, so changed the way of installation instead of remove it.

tswast · Jan 21, 2025

noxfile.py

+    session.install(
+        "scikit-learn>=1.2.2",
+    )


If there is optional functionality that needs scikit-learn, please use the "extras" section in setup.py. Don't install packages this way.

tswast · Jan 21, 2025

This is not a chore: . This is a user-visible change and should be labeled as such.

tswast

Please do not merge as is. We are opening our customers up to a big risk of having a broken installation by installing scikit-learn in so many of our test sessions.

tswast

You did not address my feedback. You just changed how you install scikit-learn. You're still installing scikit-learn in way too many of our test sessions.

tswast · Feb 21, 2025

noxfile.py

+    "3.9": ["scikit-learn"],
+    "3.10": ["scikit-learn"],
+    "3.11": ["scikit-learn"],
+    "3.12": ["polars", "scikit-learn"],
+    "3.13": ["scikit-learn"],


No.

We can't install it in every session. We need to know that bigframes works without this package!

Suggested change

"3.9": ["scikit-learn"],

"3.10": ["scikit-learn"],

"3.11": ["scikit-learn"],

"3.12": ["polars", "scikit-learn"],

"3.13": ["scikit-learn"],

"3.12": ["polars", "scikit-learn"],

Updated, but I assume in this case unit noextras sessions runs the same thing for 3.13?

tswast · Feb 21, 2025

noxfile.py

@@ -96,7 +102,7 @@
 ]
 SYSTEM_TEST_LOCAL_DEPENDENCIES: List[str] = []
 SYSTEM_TEST_DEPENDENCIES: List[str] = []
-SYSTEM_TEST_EXTRAS: List[str] = ["tests"]
+SYSTEM_TEST_EXTRAS: List[str] = ["tests", "scikit-learn"]


No.

We can't install it in every session. We need to know that bigframes works without this package! Use SYSTEM_TEST_EXTRAS_BY_PYTHON, instead.

tswast · Feb 21, 2025

noxfile.py

@@ -510,7 +515,7 @@ def docs(session):
 def docfx(session):
    """Build the docfx yaml files for this library."""

-    session.install("-e", ".")
+    session.install("-e", ".[scikit-learn]")


I'm very concerned if we need to install scikit-learn just to build the docs. Please remove.

Suggested change

session.install("-e", ".[scikit-learn]")

session.install("-e", ".")

Not sure if it's possible to remove this as we are calling publish_api_coverage.py, and one of the things it do is go through sklearn APIs and generate a coverage report.

tswast · Feb 21, 2025

noxfile.py

@@ -468,8 +474,7 @@ def cover(session):
 @nox.session(python=DEFAULT_PYTHON_VERSION)
 def docs(session):
    """Build the docs for this library."""
-
-    session.install("-e", ".")
+    session.install("-e", ".[scikit-learn]")


I'm very concerned if we need to install scikit-learn just to build the docs. Please remove.

Suggested change

session.install("-e", ".[scikit-learn]")

session.install("-e", ".")

Not sure if it's possible to remove this as we are calling publish_api_coverage.py, and one of the things it do is go through sklearn APIs and generate a coverage report.

Good point. Thanks for clarifying.

Co-authored-by: Tim Sweña (Swast) <swast@google.com>

chore: remove unused dependencies.

7001f96

product-auto-label bot added size: s Pull request size is small. api: bigquery Issues related to the googleapis/python-bigquery-dataframes API. labels Jan 16, 2025

Genesis929 added 6 commits January 16, 2025 23:10

remove dependency from test constrains.

fc52332

update test dependencies.

8d0a8f4

update nox dependencies

a5bd03d

update

605aca0

update trapz

7c487b4

update value error

3e81688

Genesis929 requested a review from GarrettWu January 17, 2025 20:54

Genesis929 and others added 2 commits January 17, 2025 12:54

Merge branch 'main' into dep_removal_huanc

c66593a

remove todo

c36fb8e

Genesis929 marked this pull request as ready for review January 17, 2025 21:14

Genesis929 requested review from a team as code owners January 17, 2025 21:14

blunderbuss-gcf bot assigned sycai Jan 17, 2025

Genesis929 added the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 17, 2025

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 17, 2025

Genesis929 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 17, 2025

bigframes-bot removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 17, 2025

GarrettWu approved these changes Jan 17, 2025

View reviewed changes

Genesis929 added 2 commits January 17, 2025 22:08

update dep

ab18fe0

update test env

ae17f8b

tswast reviewed Jan 21, 2025

View reviewed changes

tswast changed the title ~~chore: remove unused dependencies.~~ deps: remove scikit-learn as a required dependency Jan 21, 2025

tswast added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 21, 2025

tswast requested changes Jan 21, 2025

View reviewed changes

Genesis929 added 2 commits February 19, 2025 22:40

Merge branch 'main' into dep_removal_huanc

f0cbe9a

code update

207a6e3

product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Feb 19, 2025

Genesis929 requested a review from tswast February 19, 2025 23:36

Genesis929 force-pushed the dep_removal_huanc branch from a14a66a to 207a6e3 Compare February 20, 2025 01:12

Genesis929 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 20, 2025

bigframes-bot removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 20, 2025

remove scipy in test

5a221ec

Genesis929 force-pushed the dep_removal_huanc branch from da02654 to 5a221ec Compare February 20, 2025 19:15

Genesis929 and others added 5 commits February 20, 2025 11:15

Merge branch 'main' into dep_removal_huanc

8f67cf7

add scipt=1.7.1 to test constraints

ab352db

update scipy version

10e6e31

update constraint

5eaeb5b

update scipy test

571ec91

Genesis929 changed the title ~~deps: remove scikit-learn as a required dependency~~ deps: remove scikit-learn and sqlalchemy as required dependencies Feb 20, 2025

Genesis929 and others added 2 commits February 21, 2025 03:17

skip some tests when sklearn doesn't exists

c0b2df3

Merge branch 'main' into dep_removal_huanc

2aa51b5

tswast requested changes Feb 21, 2025

View reviewed changes

Genesis929 and others added 4 commits February 21, 2025 10:06

Remove sklearn for unit except 3.12

3ec09cb

Co-authored-by: Tim Sweña (Swast) <swast@google.com>

update auc

565cda5

remove scipy again

2b61223

Merge branch 'main' into dep_removal_huanc

d41369a

tswast approved these changes Feb 21, 2025

View reviewed changes

Genesis929 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 21, 2025

Genesis929 enabled auto-merge (squash) February 21, 2025 21:26

Genesis929 merged commit fd8bc89 into main Feb 21, 2025
24 checks passed

Genesis929 deleted the dep_removal_huanc branch February 21, 2025 21:33

release-please bot mentioned this pull request Feb 21, 2025

chore(main): release 1.38.0 #1410

Merged

	session.install("-e", ".[scikit-learn]")
	session.install("-e", ".")

Search code, repositories, users, issues, pull requests...

deps: remove scikit-learn and sqlalchemy as required dependencies #1296

deps: remove scikit-learn and sqlalchemy as required dependencies #1296

Uh oh!

Conversation

Genesis929 commented Jan 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tswast commented Jan 21, 2025

Uh oh!

tswast left a comment

Choose a reason for hiding this comment

Uh oh!

tswast left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Genesis929 Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Genesis929 Feb 21, 2025 •

edited

Loading