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

Use the v1 version of the log api #1589

Merged
merged 1 commit into from
Nov 27, 2024
Merged

Use the v1 version of the log api #1589

merged 1 commit into from
Nov 27, 2024

Conversation

naddeoa
Copy link
Contributor

@naddeoa naddeoa commented Nov 14, 2024

This switches from the v0 to the v1 profile upload api, replacing the previous use of log_async in the whylabs client with log_profile. The apis are functionally the same but the dataset id and the org id are specified differently. The dataset id has to be supplied via a different name and ends up as a header in the request, while the org id is not specified at all because its derived from the api key.

In terms of implementation, I tried to do this all from outside of whylogs to avoid having to make this change but there are several issues that pop up when you try to pass in your own WhylabsClient, and especially your own ApiClient that made that impossible. The refactor looks daunting as well.

Instead, this change just passes on the WhylabsClient to the other "sub writers" that are secretly made in the whylabs writer, and hard codes the use of the v1 api over the v0 api.

@richard-rogers
Copy link
Contributor

Some of the integration tests are failing. Investigating...

@richard-rogers
Copy link
Contributor

Some of the integration tests are failing. Investigating...

Some just look like the mocks for injecting throttling need to be adjusted for V1.
More concerning, some fail to retrieve the uploaded profiles. That could just be the platform running slow or something more serious.

There are also some transaction test failures I haven't looked at yet.

@richard-rogers
Copy link
Contributor

There are integration tests failing on mainline too, so maybe some of this is a backend problem.

@naddeoa
Copy link
Contributor Author

naddeoa commented Nov 18, 2024

@richard-rogers Things are all green now so I guess you reran them?

@richard-rogers
Copy link
Contributor

@richard-rogers Things are all green now so I guess you reran them?

The CI doesn't run the integration tests. They're still failing here and on mainline.

@richard-rogers
Copy link
Contributor

@richard-rogers Things are all green now so I guess you reran them?

The CI doesn't run the integration tests. They're still failing here and on mainline.

Mainline CI is passing now. I updated the URI mocks, so now only a few transaction tests are failing.

@naddeoa
Copy link
Contributor Author

naddeoa commented Nov 19, 2024

@richard-rogers can you post the failures or something so I can debug?

@richard-rogers
Copy link
Contributor

@richard-rogers can you post the failures or something so I can debug?

FAILED tests/api/writer/test_whylabs_integration.py::test_transaction_aborted - AssertionError: assert '<ExceptionIn...ntextmanager>' == 'Transaction has been aborted'
FAILED tests/api/writer/test_whylabs_integration.py::test_old_transaction_context - AssertionError: assert 1 == 7
FAILED tests/api/writer/test_whylabs_integration.py::test_transaction_distributed - IndexError: list index out of range

I will investigate them today. You can run them by setting up the WHYLABS_* environment variables and running poetry run pytest tests/api/writer/test_whylabs_integration.py --load

@richard-rogers
Copy link
Contributor

I will investigate them today. You can run them by setting up the WHYLABS_* environment variables and running poetry run pytest tests/api/writer/test_whylabs_integration.py --load

I think I figured it out. The WhyLabsTransactionWriter ctor used to always generate a new transaction id and stick it into its new WhyLabsClient. With this PR, the transaction writer is sharing the client with the parent writer, so the actual transaction id gets stomped on. I think there's a relatively easy fix, but I'd really like to clean up the writer code more in whylogs 2.0.

richard-rogers
richard-rogers previously approved these changes Nov 21, 2024
Copy link
Contributor

@richard-rogers richard-rogers left a comment

Choose a reason for hiding this comment

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

Integration tests are green now

@naddeoa naddeoa merged commit f123399 into mainline Nov 27, 2024
14 checks passed
@naddeoa naddeoa deleted the log-api-update branch November 27, 2024 19:52
richard-rogers added a commit that referenced this pull request Nov 27, 2024
richard-rogers added a commit that referenced this pull request Dec 3, 2024
## Description

Test and transaction updates to get integration tests passing again.

- [ ] I have reviewed the [Guidelines for Contributing](CONTRIBUTING.md)
and the [Code of Conduct](CODE_OF_CONDUCT.md).
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.

2 participants