-
Notifications
You must be signed in to change notification settings - Fork 895
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
PF Recording/Replaying part for sdk_cli #719
Conversation
SDK CLI Test Result yigao/recording_draft320 tests 308 ✔️ 23m 16s ⏱️ For more details on these failures, see this check. Results for commit d3b4c4a. ♻️ This comment has been updated with latest results. |
|
||
def mock_bulkresult_get_openai_metrics(self): | ||
# Some tests request the metrics in replay mode. | ||
total_metrics = {"total_tokens": 0, "duration": 0} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this return a constant ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since our tool doesn't have a real llm connection, we need to mock these returns.
src/promptflow/tests/sdk_cli_test/recording_utilities/tool_record.py
Outdated
Show resolved
Hide resolved
src/promptflow/tests/sdk_cli_test/recording_utilities/tool_record.py
Outdated
Show resolved
Hide resolved
src/promptflow/tests/sdk_cli_test/recording_utilities/tool_record.py
Outdated
Show resolved
Hide resolved
src/promptflow/tests/test_configs/flows/web_classification/webClassification20.csv
Show resolved
Hide resolved
result = {} | ||
for n in connection_names: | ||
try: | ||
conn = client.connections.get(name=n, with_secrets=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to mock this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good Point. After replaying, this is useless.
return total_metrics | ||
|
||
|
||
def mock_toolresolver_resolve_tool_by_node(recording_file: Path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add docstirng for why we need these fucntions
Description
Please add an informative description that covers that changes made by the pull request and link all relevant issues.
Recording functionalities
Generated recording files will save in test_config/node_recordings folder
Some tricks
Time estimation
166 Tests run in 3min51s, total 226 tests estimate 5min18s
Most of the time is from fetching URLs.
About techniques of copying method and inject:
Currently I didn't get good ways to reuse the original method and make some patches to the method. All these mocked functions are copied and changed separately.
All Promptflow Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines