Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate sklearn instrumentation #2708

Merged
merged 2 commits into from
Jul 16, 2024
Merged

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Jul 15, 2024

Fixes #2176

@ocelotl ocelotl added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 15, 2024
@ocelotl ocelotl self-assigned this Jul 15, 2024
@ocelotl ocelotl requested a review from a team July 15, 2024 23:04
@ocelotl ocelotl removed the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 15, 2024
@xrmx
Copy link
Contributor

xrmx commented Jul 16, 2024

This is more like remove than deprecate, I was thinking that we may drop it when we remove support for python 3.8 maybe?

@ocelotl
Copy link
Contributor Author

ocelotl commented Jul 16, 2024

This is more like remove than deprecate, I was thinking that we may drop it when we remove support for python 3.8 maybe?

This is deprecation. We no longer will publish it, but the sklearn instrumentation packages that have been published so far will continue to exist. Supporting 3.8 or not is a topic unrelated to this PR, the reason we want to deprecate this instrumentation is because the sklearn package is deprecated:

Starting 2023 December 1st, trying to install the sklearn PyPI package will raise an error.

@lzchen
Copy link
Contributor

lzchen commented Jul 16, 2024

This is more like remove than deprecate, I was thinking that we may drop it when we remove support for python 3.8 maybe?

This is deprecation. We no longer will publish it, but the sklearn instrumentation packages that have been published so far will continue to exist. Supporting 3.8 or not is a topic unrelated to this PR, the reason we want to deprecate this instrumentation is because the sklearn package is deprecated:

Starting 2023 December 1st, trying to install the sklearn PyPI package will raise an error.

@ocelotl

Might want to include this in the CHANGELOG.md that this is deprecated at least.

@xrmx

We do have precedence of simply deleting packages when they are being deprecated, although, we should probably be more transparent to users. #1366

Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove the package from the CI in the core repo?

@xrmx
Copy link
Contributor

xrmx commented Jul 16, 2024

@xrmx

We do have precedence of simply deleting packages when they are being deprecated, although, we should probably be more transparent to users. #1366

You are breaking current opentelemetry-bootstrap users no? It won't install the package anymore.

BTW it's not that I care this specific package but would like to avoid surprises downstream :) boto instrumentation probably is in an even worse state.

@ocelotl
Copy link
Contributor Author

ocelotl commented Jul 16, 2024

@xrmx
We do have precedence of simply deleting packages when they are being deprecated, although, we should probably be more transparent to users. #1366

You are breaking current opentelemetry-bootstrap users no? It won't install the package anymore.

BTW it's not that I care this specific package but would like to avoid surprises downstream :) boto instrumentation probably is in an even worse state.

Trying to install sklearn already raises an error, so I don't think this is actually breaking.

@ocelotl
Copy link
Contributor Author

ocelotl commented Jul 16, 2024

@ocelotl

Might want to include this in the CHANGELOG.md that this is deprecated at least.

It is already added.

@ocelotl ocelotl merged commit 5a7935f into open-telemetry:main Jul 16, 2024
377 checks passed
@ocelotl ocelotl deleted the issue_2176 branch July 16, 2024 20:20
@ocelotl
Copy link
Contributor Author

ocelotl commented Jul 16, 2024

Should we remove the package from the CI in the core repo?

I'll do that ✌️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop sklearn instrumentation
4 participants