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

Fugue Integration #839

Merged
merged 34 commits into from
Sep 20, 2022
Merged

Fugue Integration #839

merged 34 commits into from
Sep 20, 2022

Conversation

goodwanghan
Copy link
Contributor

@goodwanghan goodwanghan commented Sep 18, 2022

Description

This repository needs to integrate with Fugue so whyllogs profiling can work on Spark, Dask and Ray agnostically.

The commits in this pull request will:

  1. Finish the initial implementation of profiling with/without logical partitioning
  2. Docs and instructions

python/pyproject.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@TheMellyBee TheMellyBee left a comment

Choose a reason for hiding this comment

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

I have a couple of questions and some comments around cleaning it up and clarity. Over all though it's looking in a good direction.

python/pyproject.toml Outdated Show resolved Hide resolved
python/examples/integrations/Fugue_Profiling.ipynb Outdated Show resolved Hide resolved
python/docs/datasets Show resolved Hide resolved
python/whylogs/api/fugue/profiler.py Outdated Show resolved Hide resolved
python/whylogs/api/fugue/profiler.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jamie256 jamie256 left a comment

Choose a reason for hiding this comment

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

This approach looks really nice to integrate whylogs and fugue! Let me know how I can help.

This looks close, I think with some minor cleanup if you have your example notebook working, and the CI checks passing we can merge this? I will check tomorrow morning, and happy to help resolve issues.

@goodwanghan
Copy link
Contributor Author

Thank @jamie256 the notebook is working. I will also add some comments on the public function. Then we are good to merge. Currently I am having a hard time with poetry I am hitting this issue python-poetry/poetry#4242 and may try poetry 1.2 which fixes it.

@jamie256
Copy link
Contributor

I am hitting this issue python-poetry/poetry#4242 and may try poetry 1.2 which fixes it.

I updated poetry to version 1.2.1 in our CI configs as it now works after fixing the typing-extensions entry: #847

Han Wang and others added 2 commits September 20, 2022 05:04
* update poetry version in ci configs, and update poetry.lock with poetry 1.2.1
@goodwanghan
Copy link
Contributor Author

I am hitting this issue python-poetry/poetry#4242 and may try poetry 1.2 which fixes it.

I updated poetry to version 1.2.1 in our CI configs as it now works after fixing the typing-extensions entry: #847

Thanks, but please let me do that, I am making a couple of changes

@goodwanghan
Copy link
Contributor Author

Merging poetry.lock is not fun 😢

@jamie256
Copy link
Contributor

Merging poetry.lock is not fun 😢

Looks like you have it working! And notebooks cleaned up.

poetry.lock should be generated from pyproject.toml file changes by running poetry update (if possible), manually updating the lock file is painful.

Copy link
Contributor

@jamie256 jamie256 left a comment

Choose a reason for hiding this comment

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

Thanks for the push and all the updates here, I'll pull this into today's release.

Copy link
Contributor

@andyndang andyndang left a comment

Choose a reason for hiding this comment

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

lgtm!

@jamie256 jamie256 merged commit d024904 into whylabs:mainline Sep 20, 2022
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.

4 participants