cost tracking for otel_tracing #1569
Conversation
dosubot
bot
added
the
size:L
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
sfc-gh-pmardziel
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 6e400c8 in 30 seconds
More details
- Looked at
267lines of code in7files - Skipped
2files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. src/core/trulens/experimental/otel_tracing/core/app.py:20
- Draft comment:
Remove the TODO comment as it is not necessary and does not add value to the code. - Reason this comment was not posted:
Confidence changes required:10%
The comment is not necessary as the code is self-explanatory. The comment is also not consistent with the rest of the codebase where such comments are not used.
2. src/core/trulens/experimental/otel_tracing/core/app.py:139
- Draft comment:
Remove the comment as it is not necessary and does not add value to the code. - Reason this comment was not posted:
Confidence changes required:10%
The comment is not necessary as the code is self-explanatory. The comment is also not consistent with the rest of the codebase where such comments are not used.
3. src/core/trulens/experimental/otel_tracing/core/app.py:165
- Draft comment:
Remove the comment as it is not necessary and does not add value to the code. - Reason this comment was not posted:
Confidence changes required:10%
The comment is not necessary as the code is self-explanatory. The comment is also not consistent with the rest of the codebase where such comments are not used.
Workflow ID: wflow_UnvBrRMeILGV5iei
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
dosubot
bot
added
size:XXL
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to e491efe in 55 seconds
More details
- Looked at
4241lines of code in31files - Skipped
3files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. src/providers/bedrock/trulens/providers/bedrock/endpoint.py:158
- Draft comment:
Ensurebodyis not astrbefore iterating, asstris aSequencebut not iterable in the intended way. This could lead to unexpected behavior ifbodyis a string. - Reason this comment was not posted:
Comment did not seem useful.
2. src/providers/huggingface/trulens/providers/huggingface/endpoint.py:61
- Draft comment:
Consider normalizing the URL or using a more flexible check to ensure all valid Huggingface API calls are tracked, even if they have trailing slashes or different subdomains. - Reason this comment was not posted:
Confidence changes required:50%
Insrc/providers/huggingface/trulens/providers/huggingface/endpoint.py, theon_callable_callmethod in_WrapperHuggingfaceEndpointCallbackclass checks if the URL starts withhttps://api-inference.huggingface.co. However, it does not handle cases where the URL might have trailing slashes or different subdomains. This could lead to missed tracking for valid Huggingface API calls.
3. src/providers/litellm/trulens/providers/litellm/endpoint.py:84
- Draft comment:
Ensure theusagefield exists in the response before accessing it to prevent potential errors. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_wnVNXYRCiUCiqCtN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
src/providers/huggingface/trulens/providers/huggingface/provider.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5fcc28c in 46 seconds
More details
- Looked at
432lines of code in20files - Skipped
1files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. tests/e2e/test_context_variables.py:21
- Draft comment:
Ensure that the environment variables for Snowflake connection are set before running the tests to avoid connection issues. - Reason this comment was not posted:
Confidence changes required:50%
The test case is using an environment variable for the Snowflake account, which might not be set during test execution. This could lead to test failures if the environment is not properly configured.
2. tests/e2e/test_context_variables.py:22
- Draft comment:
Ensure that the environment variables for Snowflake connection are set before running the tests to avoid connection issues. - Reason this comment was not posted:
Confidence changes required:50%
The test case is using an environment variable for the Snowflake user, which might not be set during test execution. This could lead to test failures if the environment is not properly configured.
3. tests/e2e/test_context_variables.py:23
- Draft comment:
Ensure that the environment variables for Snowflake connection are set before running the tests to avoid connection issues. - Reason this comment was not posted:
Confidence changes required:50%
The test case is using an environment variable for the Snowflake user password, which might not be set during test execution. This could lead to test failures if the environment is not properly configured.
4. tests/e2e/test_context_variables.py:24
- Draft comment:
Ensure that the environment variables for Snowflake connection are set before running the tests to avoid connection issues. - Reason this comment was not posted:
Confidence changes required:50%
The test case is using an environment variable for the Snowflake database, which might not be set during test execution. This could lead to test failures if the environment is not properly configured.
5. tests/e2e/test_context_variables.py:25
- Draft comment:
Ensure that the environment variables for Snowflake connection are set before running the tests to avoid connection issues. - Reason this comment was not posted:
Confidence changes required:50%
The test case is using an environment variable for the Snowflake role, which might not be set during test execution. This could lead to test failures if the environment is not properly configured.
6. tests/e2e/test_context_variables.py:26
- Draft comment:
Ensure that the environment variables for Snowflake connection are set before running the tests to avoid connection issues. - Reason this comment was not posted:
Confidence changes required:50%
The test case is using an environment variable for the Snowflake warehouse, which might not be set during test execution. This could lead to test failures if the environment is not properly configured.
7. tests/e2e/test_context_variables.py:27
- Draft comment:
Ensure that the environment variables for Snowflake connection are set before running the tests to avoid connection issues. - Reason this comment was not posted:
Confidence changes required:50%
The test case is using an environment variable for the Snowflake schema, which might not be set during test execution. This could lead to test failures if the environment is not properly configured.
8. tests/e2e/test_context_variables.py:73
- Draft comment:
Consider implementing a more reliable method to verify if the context variable is cleaned up properly after test execution. - Reason this comment was not posted:
Confidence changes required:50%
The test case uses a TODO comment to indicate that a better way to check if the context variable is cleaned should be found. This indicates that the current method might not be reliable or optimal.
Workflow ID: wflow_emxz1Hm0H4mDnFrN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 24f31e4 in 41 seconds
More details
- Looked at
391lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_VcrVsgwzwS3UrZz0
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 356bb2b in 33 seconds
More details
- Looked at
145lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/feedback/trulens/feedback/dummy/endpoint.py:615
- Draft comment:
The comment is misleading. The code handles responses from the huggingface API and dummy completion API, not OpenAI completion. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment points out that the docstring was updated to reflect that the code handles responses from huggingface and a dummy completion API, not OpenAI. This aligns with the change made in the diff, which removed the mention of OpenAI and replaced it with a dummy completion API. The comment seems to be reinforcing the change made in the diff, which is valid.
I might be overthinking the necessity of the comment since the change in the docstring already clarifies the intended functionality. The comment might be redundant.
The comment serves as a confirmation that the change in the docstring aligns with the actual functionality of the code, which can be useful for clarity.
Keep the comment as it confirms the accuracy of the updated docstring, which aligns with the change made in the diff.
Workflow ID: wflow_jmO8NB7nz8wB3YcU
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 0ac6f5a in 33 seconds
More details
- Looked at
2331lines of code in54files - Skipped
23files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/providers/huggingface/trulens/providers/huggingface/provider.py:360
- Draft comment:
The_tcidecorator does not handleNonevalues for string inputs. Consider adding a check forNonebefore checking if the string is non-empty to avoid potential errors. - Reason this comment was not posted:
Confidence changes required:50%
The code insrc/providers/huggingface/trulens/providers/huggingface/provider.pyhas a potential issue with the_tcidecorator. The decorator checks for non-empty strings but does not handle cases where the input might beNone. This could lead to unexpected errors ifNoneis passed as an argument. The decorator should be updated to handleNonevalues appropriately.
Workflow ID: wflow_lt3BfNw4YXprsmtA
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| records.append(record) | ||
|
|
||
| return records | ||
| yield record |
There was a problem hiding this comment.
so I assume this is to avoid waiting on long blocking feedback computation. just curious what are some of the actual deadlock scenarios you've seen?
There was a problem hiding this comment.
I have seen deadlocks deeper in wait_for_feedback_results which I couldn't fully explain. This change is to make things a bit more robust but wouldn't really fully address deadlocks if they are still there.
| @@ -50,6 +56,15 @@ | |||
| https://www.trulens.org/component_guides/other/no_context_warning for more information. | |||
| """ | |||
|
|
|||
| _RE_NO_RETRY = re.compile( | |||
| "(" | |||
| + ("|".join(["authentication", "unauthorized", "expired", "quota"])) | |||
There was a problem hiding this comment.
does this cover invalid (400 error code) type of errors (or are those not relevant / not available as Exception here) ?
There was a problem hiding this comment.
I'm unsure what error codes are associated with the text checks I did. Error codes might be a more robust approach than looking at the text as is implemented now. Can we make that a todo?
| session = core_session.TruSession() | ||
|
|
||
| if session.experimental_feature( | ||
| core_experimental.Feature.OTEL_TRACING, lock=True |
There was a problem hiding this comment.
nit on semantics: can we rename the parameter lock to something like locked, to reflect it being a boolean?
when I read lock=True I would think of the lock primitive for synchronization, instead of locking the feature / making the feature immutable.
Open to suggestions other than "locked", and not meant to be a blocking comment.
There was a problem hiding this comment.
Renamed to "freeze" and "frozen".
| ) -> Dict: | ||
| """Wraps `post` with json()[0].""" | ||
|
|
||
| return self.post(url=url, json=json, timeout=timeout).json()[0] |
There was a problem hiding this comment.
can the response returned by requests.post be empty list / not a list and result in indexing error at json()[0]?
There was a problem hiding this comment.
added checks for this
| ) -> Dict: | ||
| """Wraps `apost` with json()[0].""" | ||
|
|
||
| return (await self.apost(url=url, json=json, timeout=timeout)).json()[0] |
There was a problem hiding this comment.
same as above, could we handle the potential index error for json()[0] (or verify if this won't ever happen)?
|
|
||
| else: | ||
| # Acquire was used with blocking=False and the lock would have | ||
| # blocked. |
There was a problem hiding this comment.
shouldn't this read sth like
Acquire was used with blocking=False and should return immediately if the lock is not yet released?
or maybe I'm misunderstanding the comment here
There was a problem hiding this comment.
I think we are both saying the same thing. Added some more text to maybe unify.
| "mode": "completion", | ||
| "model": model, | ||
| "prompt": prompt, | ||
| "temperature": temperature, | ||
| "args": args, # include extra args to see them in post span | ||
| }, | ||
| ) | ||
| ).json()[0] |
There was a problem hiding this comment.
same concern as in other instances of using json()[0] - could we verify if indexing error is not going to happen here?
There was a problem hiding this comment.
_fake_completion and _fake_classification are the only status 200 returns in DummyAPI so here we can be sure of [0] existing. Added a check for status code just in case post returns without raising error on non-200 statuses.
| "mode": "classification", | ||
| "model": model, | ||
| "inputs": text, | ||
| "args": args, # include extra args to see them in post span | ||
| }, | ||
| ) | ||
| ).json()[0] |
There was a problem hiding this comment.
see above comments regarding json()[0]
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5c73258 in 51 seconds
More details
- Looked at
278lines of code in5files - Skipped
1files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. tests/e2e/test_snowflake_notebooks.py:36
- Draft comment:
Consider using a context manager fortempfile.TemporaryDirectory()to ensure the temporary directory is always cleaned up, even if an exception occurs.
with tempfile.TemporaryDirectory() as tmp_directory:
- Reason this comment was not posted:
Confidence changes required:50%
The code intest_snowflake_notebooks.pyusestempfile.TemporaryDirectory()but does not handle the case where the directory might not be cleaned up if an exception occurs before thefinallyblock. Using a context manager would ensure that the directory is always cleaned up.
2. src/connectors/snowflake/trulens/connectors/snowflake/utils/server_side_evaluation_artifacts.py:23
- Draft comment:
Consider automating the retrieval of package versions for_TRULENS_PACKAGES_DEPENDENCIESto ensure they are always up-to-date. - Reason this comment was not posted:
Confidence changes required:50%
Inserver_side_evaluation_artifacts.py, the_TRULENS_PACKAGES_DEPENDENCIESlist is hardcoded. It would be better to automate the retrieval of these package versions to ensure they are always up-to-date.
3. src/core/trulens/core/database/sqlalchemy.py:548
- Draft comment:
The workaround for handlingNonevalues in Snowflake should be documented more clearly, and consider seeking a more robust solution if possible. - Reason this comment was not posted:
Confidence changes required:50%
Insqlalchemy.py, theinsert_feedbackmethod uses a hack for Snowflake to handleNonevalues. This workaround should be documented more clearly, and ideally, a more robust solution should be sought.
Workflow ID: wflow_GxZ8F2JVeTccI0O1
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c9e2c13 in 1 minute and 13 seconds
More details
- Looked at
701lines of code in9files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. src/core/trulens/core/experimental/__init__.py:143
- Draft comment:
The term 'lock' has been consistently replaced with 'freeze' throughout this file. This change aligns the terminology with the intended functionality of preventing further changes to a setting once it is set. Ensure that this semantic change is communicated to all relevant stakeholders to avoid confusion. - Reason this comment was not posted:
Confidence changes required:10%
The change from 'lock' to 'freeze' is consistent and aligns with the intended functionality of preventing further changes to a setting once it is set. This is a semantic change that should be communicated to all relevant stakeholders to avoid confusion.
2. src/core/trulens/core/feedback/endpoint.py:572
- Draft comment:
The term 'lock' has been replaced with 'freeze' to better reflect the functionality of preventing further changes to a setting once it is set. This change is consistent with the rest of the codebase. - Reason this comment was not posted:
Confidence changes required:10%
The change from 'lock' to 'freeze' is consistent and aligns with the intended functionality of preventing further changes to a setting once it is set. This is a semantic change that should be communicated to all relevant stakeholders to avoid confusion.
3. src/core/trulens/core/instruments.py:573
- Draft comment:
The term 'lock' has been replaced with 'freeze' to better reflect the functionality of preventing further changes to a setting once it is set. This change is consistent with the rest of the codebase. - Reason this comment was not posted:
Confidence changes required:10%
The change from 'lock' to 'freeze' is consistent and aligns with the intended functionality of preventing further changes to a setting once it is set. This is a semantic change that should be communicated to all relevant stakeholders to avoid confusion.
4. src/core/trulens/experimental/otel_tracing/core/session.py:27
- Draft comment:
The term 'lock' has been replaced with 'freeze' to better reflect the functionality of preventing further changes to a setting once it is set. This change is consistent with the rest of the codebase. - Reason this comment was not posted:
Confidence changes required:10%
The change from 'lock' to 'freeze' is consistent and aligns with the intended functionality of preventing further changes to a setting once it is set. This is a semantic change that should be communicated to all relevant stakeholders to avoid confusion.
5. src/core/trulens/experimental/otel_tracing/core/trace.py:433
- Draft comment:
The term 'lock' has been replaced with 'freeze' to better reflect the functionality of preventing further changes to a setting once it is set. This change is consistent with the rest of the codebase. - Reason this comment was not posted:
Confidence changes required:10%
The change from 'lock' to 'freeze' is consistent and aligns with the intended functionality of preventing further changes to a setting once it is set. This is a semantic change that should be communicated to all relevant stakeholders to avoid confusion.
6. src/feedback/trulens/feedback/dummy/endpoint.py:406
- Draft comment:
The change to check the response status code and raise an error if it's not 200 is a good addition for error handling. This ensures that unexpected responses are caught early. - Reason this comment was not posted:
Confidence changes required:10%
The change from 'lock' to 'freeze' is consistent and aligns with the intended functionality of preventing further changes to a setting once it is set. This is a semantic change that should be communicated to all relevant stakeholders to avoid confusion.
Workflow ID: wflow_qC2dfXGy8F7eCPTC
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on c07ec92 in 53 seconds
More details
- Looked at
13lines of code in1files - Skipped
1files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_8xhz56k3NCOVX6OV
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -182,6 +180,8 @@ def experimental_otel_exporter( | |||
| def experimental_otel_exporter( | |||
| self, value: Optional[Any] | |||
| ): # Any = otel_export_sdk.SpanExporter | |||
| otel_tracing_feature._FeatureSetup.assert_optionals_installed() | |||
There was a problem hiding this comment.
Accessing a private method _FeatureSetup.assert_optionals_installed() is not recommended as it can lead to maintenance issues. Consider using a public interface instead.
There was a problem hiding this comment.
@sfc-gh-pmardziel Could you document the progress of implementing each Endpoint class in the PR description (i.e. Huggingface is done vs OpenAI is WIP vs Cortex is not started), so that we could pick it up and finish them in the following week?
Did this. |
* Bump version to 1.2.6. (#1635) * Bump version to 1.2.6. * Update `meta.yaml` files for conda packages. * cost tracking for otel_tracing (#1569) * cleaning up old PR * adding cost tracking to dummy endpoint * finishing dummy and starting bedrock * finish up * replacement for track_all_costs_tally * docs * fixing up dummy api * added huggingface cost tracking for otel * working on awaitables * docs * debugging async issue * work * async parallel fixes * nit * fix optional imports handling with fromlist * remove print * remove another print * doc format * put snowflake env var check back in test_endpoints * remove context.run * make feature private * making more things private * add type args * nits * addressing PR comments * typo * example: context filters with different speeds (models) (#1633) * context filters with different speeds * drop dup cell * titling * add all deps --------- Co-authored-by: Josh Reini <[email protected]> * fix docs typo about aggregate (#1640) * Add Datec logo to homepage (#1643) * datec logo * update link --------- Co-authored-by: Josh Reini <[email protected]> * Backport TruLens to Python 3.8.1 (#1644) * backport some packages to 3.8 * add to testing matrix * fix optional test condition * drop everything to 3.8 and resolve versioning issues * use importlib-resources for 3.8 compatibility * unpin langchain versions * clean * add future annotations * more test fixes * fix required test annotations * one more * fix install issues * handle missing functools.cache in 3.8 * don't skip optional tests on 3.8 * support llama-index/tiktoken dep in 3.13 * disable 3.13 tests * fix test: nemoguardrails supported in 3.12 * add restricted langchain versions * If the Snowflake connector supplied to us has a "pyformat" paramstyle, fail fast and give helpful advice to fix it. (#1637) * Remove all but root `poetry.lock` file. (#1653) * fix missing description (#1645) * change instrumentation arg `self` -> `_self` (#1656) * change self -> _self * note update * Make Dashboard fully Streamlit 1.35.0 Native (SiS Compatible) (#1648) * add optional flag for disabling non-native streamlit dependencies * use feature flag and dependency update * fix flag * hide record_viewer on compat mode enabled * update pills * use streamlit pills if available * wrap dashboard page entrypoints in fn * hide record viewer here * add compat utils for >=1.35 compatibility * container compatibility * fix st.code and pills code * add dashboard to zip-wheels * stage on sf and stuff * fix ipywidgets compatible version for streamlit conflicts * safe checks for query param key * cleanup leaderboard * cleanup artifact upload * update golden api * cleanup old recipe * update comment * fix langchain build issue * update locks * fix environment.yml creation * update meta.yaml for other packages * pr fixes * cant use different database/schema from pkg stage * revert examples change * fmt * fix server-side eval tests * DRY sis dashboard code * fix run_query typing * default to native streamlit if packages not installed * stage utils * parameterize streamlit app name * add sis_utils for sis dashboard * add module string * sis from snowflake connector init * dataframe column filtering * arg order * display record and app json in place of record viewer * wrap import in optional import * wrap trulens.connectors.snowflake * use native dataframe if st_aggrid is not installed * api * version bump * ignore locks --------- Co-authored-by: David Kurokawa <[email protected]> Co-authored-by: Piotr Mardziel <[email protected]> Co-authored-by: Josh Reini <[email protected]> Co-authored-by: Josh Reini <[email protected]> Co-authored-by: Daniel Huang <[email protected]>
Description
OTEL cost tracing
Adding cost tracking to the experimental otel-based tracing for these providers:
HuggingfaceDummyHuggingfaceDummyPart of the code for the other endpoints is included but rest needs to be brought in from #1168 :
Wrapper class refinement
Updated the classes used to wrap instrumented functions being used for both cost tracking and record information. The classes that store various handlers now have type variables indicating what they expect on the handler signatures.
Experimental feature optional modules
Added template for dealing with optional modules required for experimental features. Initial demonstration is for otel_tracing in
trulens.experimental.otel_tracing._feature.pywhich is hooked into the feature disable/enable system so that:Will print out the following if the packages needed for this feature are not installed:
Smaller things
OptionalImportiffromlistis used.dummy_example.ipynb. Included now are parallel invocations of apps using both threads and async.App.wait_for_feedback_results.Endpointmethods that retry request to not retry them for particular types of errors like those indicating misconfiguration (bad authentication, etc.).Endpointas implemented byPace.Endpointinto a mixin calledWithPost. Modified thepostmethod to have identical signature torequests.post.Costclass fields to better indicate their interpretation.Record.experimental_otel_spansbe properly serializable and deserializable meaning it is now available for deferred feedback.test_endpoints.pythat enables theotel_tracingexperimental feature when running the tests.Other details good to know for developers
Please include any other details of this change useful for TruLens developers.
Type of change
not work as expected)
Important
Add cost tracking for OTEL-based tracing in Huggingface, DummyHuggingface, and Dummy providers, with refinements to wrapper classes and experimental feature support.
Huggingface,DummyHuggingface, andDummyproviders._WrapperEndpointCallbackincore/feedback/endpoint.pyfor cost tracking.LiveSpanCallWithCostincore/trace.pyto include cost tracking.feature.pyfor experimental features likeotel_tracing.test_endpoints.py.core/_utils/wrap.py.core/utils/imports.py.core/schema/base.py,core/schema/record.py,core/schema/select.py, andcore/session.pyto reflectEXPERIMENTAL(otel_tracing).core/app.pyandcore/instruments.py.README.md.This description was created by
for c07ec92. It will automatically update as commits are pushed.