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.

Sign up for GitHub

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

Jump to bottom

Replace snappy with cramjam #1091

Merged
merged 5 commits into from
Mar 14, 2024
Merged

Replace snappy with cramjam #1091

merged 5 commits into from
Mar 14, 2024

Conversation

gliptak
Copy link
Contributor

@gliptak gliptak commented Feb 2, 2024

No description provided.

@gliptak gliptak requested a review from a team as a code owner February 2, 2024 20:15
@gliptak gliptak mentioned this pull request Feb 2, 2024
Merged
@gliptak
Copy link
Contributor Author

gliptak commented Feb 5, 2024

@taylorfturner please assign reviewers

@taylorfturner
Copy link
Contributor

taylorfturner commented Feb 5, 2024 •
edited
Loading

Thanks for your patience @gliptak -- we will review your proposal

@gliptak
Copy link
Contributor Author

gliptak commented Feb 20, 2024

please review

taylorfturner
taylorfturner reviewed Feb 21, 2024
View reviewed changes
Copy link
Contributor

@taylorfturner taylorfturner left a comment

Choose a reason for hiding this comment

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

I need to do some additional research on this as tests are not passing on the following lines locally, which is odd to me:

I like the change generally just need some additional research on this to ensure we are covering all the bases. @gliptak Thanks

@gliptak
Copy link
Contributor Author

gliptak commented Feb 26, 2024

capitalone/synthetic-data#346

@taylorfturner taylorfturner changed the base branch from old-old-dev to dev March 7, 2024 12:28
@taylorfturner
Copy link
Contributor

@gliptak rebase onto dev and I'll approve

@taylorfturner
Copy link
Contributor

rebase or update branch on this and I'll approve @gliptak

@gliptak
Copy link
Contributor Author

gliptak commented Mar 7, 2024

green for your review @taylorfturner

@taylorfturner taylorfturner enabled auto-merge (squash) March 8, 2024 18:04
taylorfturner
taylorfturner approved these changes Mar 8, 2024
View reviewed changes
Copy link
Contributor

@taylorfturner taylorfturner left a comment

Choose a reason for hiding this comment

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

LGTM

@gliptak
Copy link
Contributor Author

gliptak commented Mar 11, 2024

@micdavis @ksneab7 please review/approve

@taylorfturner taylorfturner merged commit f411110 into capitalone:dev Mar 14, 2024
5 checks passed
@gliptak gliptak deleted the cramjam1 branch March 14, 2024 14:33
@taylorfturner
Copy link
Contributor

taylorfturner commented Mar 22, 2024 •
edited
Loading

@gliptak are you able to restore your cramjam1 branch?

This change fails tests locally on this command DATAPROFILER_SEED=0 python3 -m unittest discover -p "test*.py"

First issues is dataprofiler/dataprofiler/tests/test_dp_logging.py", line 27, in test_default_verbosity self.assertEqual( AssertionError: 20 != 30

Second issue is dataprofiler/dataprofiler/tests/test_data_profiler.py", line 32, in test_set_seed self.assertEqual(dp.settings._seed, None) AssertionError: 0 != None

main testing works locally and the only diff between main and dev is this PR

We can either:

  • restore your branch and revert this PR
  • or I or you can create a new PR to revert back to snappy, as it was prior to this PR

@taylorfturner
Copy link
Contributor

taylorfturner commented Mar 22, 2024 •
edited
Loading

Checking out f8b3e5dbd4b76f0ecc291911ace9e8e21cf1ecb1 and running tests on that commit (one commit prior to this PR) ensures that tests did run on dev locally at that point in the history @gliptak

@taylorfturner
Copy link
Contributor

Glad to ultimately do cramjam + python 3.11 -- these tests will just need to pass locally as well as remote

@taylorfturner taylorfturner mentioned this pull request Mar 22, 2024
Merged
@gliptak gliptak restored the cramjam1 branch March 22, 2024 19:41
@gliptak
Copy link
Contributor Author

gliptak commented Mar 22, 2024

@taylorfturner restored branch

as the build was green for this PR, might that indicate that different library versions where used?

@taylorfturner
Copy link
Contributor

taylorfturner commented Mar 22, 2024 •
edited
Loading

potentially -- I tried a bunch of different scenarios and wasn't able to find a setup where the above two tests passed locally.

Did you run DATAPROFILER_SEED=0 python3 -m unittest discover -p "test*.py" on your end locally with cramjam1?

@gliptak
Copy link
Contributor Author

gliptak commented Mar 24, 2024 •
edited
Loading

I did reproduce the fail locally and still cannot reproduce in GHA

the one difference I observed is that local is Python 3.10.12 while GHA is CPython (3.10.13)

abajpai15 pushed a commit to abajpai15/DataProfiler that referenced this pull request Apr 1, 2024
@gliptak @taylorfturner @abajpai15
Replace snappy with cramjam (capitalone#1091)
b301dd9 
* add downloads tile (capitalone#1085)

* Replace snappy with cramjam

* Delete test_no_snappy

---------

Co-authored-by: Taylor Turner <taylorfturner@gmail.com>
micdavis pushed a commit that referenced this pull request May 20, 2024
@taylorfturner @gliptak
@abajpai15 @carlsonp @SchadtJ
Version.py update 0.11.0 (#1139)
a448694 
* Replace snappy with cramjam (#1091)

* add downloads tile (#1085)

* Replace snappy with cramjam

* Delete test_no_snappy

---------

Co-authored-by: Taylor Turner <taylorfturner@gmail.com>

* Quick fix for dependency max pins (#1120)

* Fix dask_expr

* Keras and Tensorflow version fix

* Keras and Tensorflow version fix

* Fix keras bug

* pre-commit fix (#1122)

* docs: update test link to latest version (#1114)

* docs: add contributor notes on where to find documentation branches (#1113)

* docs: add contributor notes on where to find documentation branches

* docs: update documentation wording to spell out why `dev-gh-pages` and `gh-pages` branches exist for staging content

* docs: add note on fork

Co-authored-by: Taylor Turner <taylorfturner@gmail.com>

* Update .github/CONTRIBUTING.md

Co-authored-by: Taylor Turner <taylorfturner@gmail.com>

---------

Co-authored-by: Taylor Turner <taylorfturner@gmail.com>

* update black version (#1131)

* Add memray max version (#1132)

* Bug fix for float precision calculation using categorical data with trailing zeros. (#1125)

* Revert "Bug fix for float precision calculation using categorical data with t…" (#1133)

This reverts commit d3159bd.

* fix

* make up to date

* yep, shouldn't change

* bump version

---------

Co-authored-by: Gábor Lipták <gliptak@gmail.com>
Co-authored-by: abajpai15 <73404357+abajpai15@users.noreply.github.com>
Co-authored-by: Patrick Carlson <carlson2442@gmail.com>
Co-authored-by: James Schadt <jamesrschadt@gmail.com>
JGSweets pushed a commit to JGSweets/data-profiler that referenced this pull request Jun 5, 2024
@gliptak @taylorfturner @JGSweets
Replace snappy with cramjam (capitalone#1091)
318b0b0 
* add downloads tile (capitalone#1085)

* Replace snappy with cramjam

* Delete test_no_snappy

---------

Co-authored-by: Taylor Turner <taylorfturner@gmail.com>
taylorfturner added a commit that referenced this pull request Jun 6, 2024
@JGSweets @gliptak
@taylorfturner @SchadtJ
refactor: Upgrade the models to use keras 3.0 (#1138)
a909a00 
* Replace snappy with cramjam (#1091)

* add downloads tile (#1085)

* Replace snappy with cramjam

* Delete test_no_snappy

---------

Co-authored-by: Taylor Turner <taylorfturner@gmail.com>

* pre-commit fix (#1122)

* Bug fix for float precision calculation using categorical data with trailing zeros. (#1125)

* Revert "Bug fix for float precision calculation using categorical data with t…" (#1133)

This reverts commit d3159bd.

* refactor: move layers outside of class

* refactor: update model to keras 3.0

* fix: manifest

* fix: bugs in compile and train

* fix: bug in load_from_library

* fix: bugs in CharCNN

* refactor: loading tf model labeler

* fix: bug in data_labeler identification

* fix: update model to use proper softmax layer names

* fix: formatting

* fix: remove unused line

* refactor: drop support for 3.8

* fix: comments

* fix: comment

---------

Co-authored-by: Gábor Lipták <gliptak@gmail.com>
Co-authored-by: Taylor Turner <taylorfturner@gmail.com>
Co-authored-by: James Schadt <jamesrschadt@gmail.com>
taylorfturner added a commit that referenced this pull request Jun 12, 2024
@gliptak @taylorfturner
Add Python 3.11 to GHA (#1090)
4e4450a 
* add downloads tile (#1085)

* Add Python 3.11 to GHA

* Replace snappy with cramjam (#1091)

* add downloads tile (#1085)

* Replace snappy with cramjam

* Delete test_no_snappy

---------

Co-authored-by: Taylor Turner <taylorfturner@gmail.com>

* Update dask modules

* Install dask dataframe

* Update dask modules in precommit

* Correct copy/paste error

* Try again to clear Unicode

* Rolled back pre-commit dask version

* Add py311 to tox

* Bump dask to 2024.4.1

* Bump python-snappy 0.7.1

* Rewrite labeler test

* Correct isort

* Satisfy black

* And flake8

* Synced with requirements

---------

Co-authored-by: Taylor Turner <taylorfturner@gmail.com>
micdavis pushed a commit that referenced this pull request Jun 14, 2024
@taylorfturner @JGSweets
@gliptak @SchadtJ
staging/main/0.12.0 (#1145)
4545841 
* refactor: Upgrade the models to use keras 3.0 (#1138)

* Replace snappy with cramjam (#1091)

* add downloads tile (#1085)

* Replace snappy with cramjam

* Delete test_no_snappy

---------

Co-authored-by: Taylor Turner <taylorfturner@gmail.com>

* pre-commit fix (#1122)

* Bug fix for float precision calculation using categorical data with trailing zeros. (#1125)

* Revert "Bug fix for float precision calculation using categorical data with t…" (#1133)

This reverts commit d3159bd.

* refactor: move layers outside of class

* refactor: update model to keras 3.0

* fix: manifest

* fix: bugs in compile and train

* fix: bug in load_from_library

* fix: bugs in CharCNN

* refactor: loading tf model labeler

* fix: bug in data_labeler identification

* fix: update model to use proper softmax layer names

* fix: formatting

* fix: remove unused line

* refactor: drop support for 3.8

* fix: comments

* fix: comment

---------

Co-authored-by: Gábor Lipták <gliptak@gmail.com>
Co-authored-by: Taylor Turner <taylorfturner@gmail.com>
Co-authored-by: James Schadt <jamesrschadt@gmail.com>

* Fix Tox (#1143)

* tox new

* update

* update

* update

* update

* update

* update

* update

* update tox.ini

* update

* update

* remove docs

* empty retrigger

* update (#1146)

* bump version

* update 3.11

* remove dist/

---------

Co-authored-by: JGSweets <JGSweets@users.noreply.github.com>
Co-authored-by: Gábor Lipták <gliptak@gmail.com>
Co-authored-by: James Schadt <jamesrschadt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@micdavis micdavis micdavis approved these changes

@taylorfturner taylorfturner taylorfturner approved these changes

@ksneab7 ksneab7 Awaiting requested review from ksneab7

Assignees

@gliptak gliptak

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gliptak @taylorfturner @micdavis