Skip to content

refactor: move from deprecated pkg_resources#1202

Open
JGSweets wants to merge 16 commits intocapitalone:mainfrom
JGSweets:fix-pkg-resource
Open

refactor: move from deprecated pkg_resources#1202
JGSweets wants to merge 16 commits intocapitalone:mainfrom
JGSweets:fix-pkg-resource

Conversation

@JGSweets
Copy link
Copy Markdown
Contributor

@JGSweets JGSweets commented Feb 10, 2026 •
edited
Loading

this pr:

  • refactors out a deprecated library that is removed in the latest setuptools
  • refactors to move resources into the DP package
  • updates setuptools to utilize include_package_data instead of data_files
  • limit pandas due to compatibility issues <3.0.0
  • aberrations between scipy versions cause slight differences past the 10th decimal. use almost to validate
    • due to cdf on p-values

@JGSweets JGSweets requested a review from a team as a code owner February 10, 2026 05:40
@JGSweets JGSweets mentioned this pull request Feb 10, 2026
Open
@JGSweets
Copy link
Copy Markdown
Contributor Author

JGSweets commented Feb 12, 2026

@ryanSoley Can we get a review on this? It will allow DP to continue working with new setuptools.

Otherwise, this library will not work in system that utilize the most up to date setuptools

@JGSweets
Copy link
Copy Markdown
Contributor Author

JGSweets commented Feb 19, 2026

@mhmotamedi any chances we could get a review on this for fixes?

shania-m
shania-m reviewed Feb 24, 2026
View reviewed changes
Comment thread dataprofiler/labelers/base_data_labeler.py Outdated
shania-m
shania-m previously approved these changes Mar 10, 2026
View reviewed changes
ryanSoley
ryanSoley previously approved these changes Mar 10, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@ryanSoley ryanSoley left a comment

Choose a reason for hiding this comment

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

thanks!

@JGSweets
Copy link
Copy Markdown
Contributor Author

JGSweets commented Mar 13, 2026

@ryanSoley @shania-m

anything that needs to be done to get the other checks to pass or merge?

Thanks!

@JGSweets JGSweets mentioned this pull request Mar 13, 2026
Open
shania-m
shania-m reviewed Mar 16, 2026
View reviewed changes
Comment thread dataprofiler/labelers/utils.py
return check_module


def find_resources_dir(resource_path: str | Path | None = None) -> Traversable:
Copy link
Copy Markdown
Contributor

@shania-m shania-m Mar 16, 2026

Choose a reason for hiding this comment

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

Is there a unit test for this function?

Copy link
Copy Markdown
Contributor Author

@JGSweets JGSweets Mar 23, 2026

Choose a reason for hiding this comment

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

it is inherently checked by many the other unit / integration tests.
Do you want one specifically for this?

Are there inputs / outputs you specifically want to test?

Copy link
Copy Markdown
Contributor Author

@JGSweets JGSweets Mar 23, 2026

Choose a reason for hiding this comment

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

FYI @shania-m

Copy link
Copy Markdown
Contributor Author

@JGSweets JGSweets Mar 30, 2026

Choose a reason for hiding this comment

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

Hi @shania-m just want to bump this convo!

Copy link
Copy Markdown
Contributor

@shania-m shania-m Mar 30, 2026

Choose a reason for hiding this comment

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

Oh but there’s no specific unit test just for the function? Is it possible to add some?

Copy link
Copy Markdown
Contributor Author

@JGSweets JGSweets Mar 30, 2026

Choose a reason for hiding this comment

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

@shania-m added!

Copy link
Copy Markdown
Contributor Author

@JGSweets JGSweets Mar 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

@shania-m FYI tests are now failing because the requirements are not limiting chardet
see: #1205

Either i need to limit in this PR or the above PR needs to merge first.

@JGSweets JGSweets dismissed stale reviews from ryanSoley and shania-m via 3d23362 March 30, 2026 17:27
@shania-m
Copy link
Copy Markdown
Contributor

shania-m commented Mar 31, 2026

Just approved the #1205 we can merge this once that’s done. Thank you!

@shania-m
Copy link
Copy Markdown
Contributor

shania-m commented Apr 7, 2026

@JGSweets please update this branch, I’ll reapprove ! Thanks

@JGSweets
Copy link
Copy Markdown
Contributor Author

JGSweets commented Apr 7, 2026

@shania-m updated! accidentally added an extra assert on the rebase which i fixed.

Also noted that the pre-commit still had the old chardet so i resolved that too

Should be ready!

shania-m
shania-m reviewed Apr 7, 2026
View reviewed changes
Comment thread dataprofiler/tests/labelers/test_data_processing.py Outdated
@@ -1,14 +1,17 @@
pass
Copy link
Copy Markdown
Contributor

@shania-m shania-m Apr 7, 2026

Choose a reason for hiding this comment

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

Why do we need this pass here?

Copy link
Copy Markdown
Contributor Author

@JGSweets JGSweets Apr 7, 2026

Choose a reason for hiding this comment

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

accidental add when working with pre-commit and swapping between branches

shania-m
shania-m reviewed Apr 7, 2026
View reviewed changes
Comment thread dataprofiler/tests/labelers/test_data_processing.py Outdated
import unittest
from io import StringIO

pass
Copy link
Copy Markdown
Contributor

@shania-m shania-m Apr 7, 2026

Choose a reason for hiding this comment

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

Same?

Copy link
Copy Markdown
Contributor Author

@JGSweets JGSweets Apr 7, 2026

Choose a reason for hiding this comment

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

accidental add when working with pre-commit and swapping between branches

@JGSweets
Copy link
Copy Markdown
Contributor Author

JGSweets commented Apr 7, 2026

@shania-m good catch, i've removed the pass and re-pushed. waiting for tests now

@JGSweets
Copy link
Copy Markdown
Contributor Author

JGSweets commented Apr 14, 2026

@shania-m all tests passed, anything else i need to resolve?

Thanks!

@JGSweets
Copy link
Copy Markdown
Contributor Author

JGSweets commented Apr 21, 2026

once this is in i'll work on the tensorflow update so we can be on the newest TF version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ryanSoley ryanSoley ryanSoley approved these changes

@shania-m shania-m shania-m approved these changes

+1 more reviewer

@zazulam zazulam zazulam approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@JGSweets @shania-m @zazulam @ryanSoley