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

fix(watsonx): exclude http request, adding span for model initialization #543

Merged
merged 1 commit into from
Feb 28, 2024
Merged

fix(watsonx): exclude http request, adding span for model initialization #543

merged 1 commit into from
Feb 28, 2024

Conversation

huang-cn
Copy link
Contributor

  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • (If applicable) I have updated the documentation accordingly.

issue related discussions in #542

@huang-cn
Copy link
Contributor Author

Here is a screenshot of E2E test:
image

WatsonxInstrumentor().instrument()
try:
# Check for required package in the env, skip test if could not found
from ibm_watsonx_ai.foundation_models import ModelInference
Copy link
Member

Choose a reason for hiding this comment

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

don't we need to add them to poetry otherwise they won't get installed in the test?

Copy link
Contributor Author

@huang-cn huang-cn Feb 28, 2024

Choose a reason for hiding this comment

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

@nirga I'm not sure if we add the ibm-watsonx-ai import in poetry, would it cause issue for test on 3.8 and 3.9? is there a way to set it to only run test on 3.10+?
in this PR #526 you mentioned adding "Python >= 3.10" will cause conflict, maybe there's other way to do this?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm we need to see if poetry supports that but I think now this test never gets run

Copy link
Contributor

Choose a reason for hiding this comment

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

@nirga test execution in CI looks good to me. It skips watsonx test on Python < 3.10, but runs normally otherwise. Traceloop SDK tests are running for all versions.
I think this is a good solution.

Copy link
Member

Choose a reason for hiding this comment

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

@paolorechia but how does it run if the Watson package is not installed? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha! It's only running the try/catch, and exiting on the except then? I missed that.

Copy link
Member

Choose a reason for hiding this comment

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

Yep 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, but at least the @huang-cn is able to run tests locally now.

Do you think we might need to revive the optional dependencies issue I created but closed? Maybe it's a way forward for these issues. I don't think it's reasonable to request the PR author to implement a fix for this though.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah let's merge it, open an issue and figure out if there's a way around it within poetry



@pytest.mark.skipif(sys.version_info < (3, 10), reason="ibm-watson-ai requires python3.10")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 nice way to skip it

WatsonxInstrumentor().instrument()
try:
# Check for required package in the env, skip test if could not found
from ibm_watsonx_ai.foundation_models import ModelInference
Copy link
Contributor

Choose a reason for hiding this comment

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

@nirga test execution in CI looks good to me. It skips watsonx test on Python < 3.10, but runs normally otherwise. Traceloop SDK tests are running for all versions.
I think this is a good solution.

@paolorechia paolorechia merged commit a2aa044 into traceloop:main Feb 28, 2024
7 checks passed
@huang-cn huang-cn deleted the watsonx_exclude_http_model_init branch February 29, 2024 02:55
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.

3 participants