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 the recordings to allow promptflow runs. #3477

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

nick863
Copy link
Contributor

@nick863 nick863 commented Jun 28, 2024

Description

The promptflow creates a lot of temporary files on-the -ground, which are being transferred to the cloud. In this PR I have added the code to sanitize the run names and added other fixes to allow run recordings.

  1. The run name and experiment name are now sanitized as well as other ID-s.
  2. Added the sanitized tracking URI in the recordings
  3. PF run first checks if the run with the same name exists and if it does it gets 404. If we continue run, we are getting two different responses to the same run name request: first 404 and then 200 (when the run was already created). These requests are the same and if failed with 404 request will be recorded, the second i.e. continuing run will fail. I have added the code to overwrite only failed runs (code >= 400)
  4. The resulting cassettes were re recorded.

See work item 3305909

All Promptflow Contribution checklist:

  • The pull request does not introduce [breaking changes].
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request to get dedicated review from promptflow team. Learn more: suggested workflow.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

Copy link

github-actions bot commented Jun 28, 2024

promptflow SDK CLI Azure E2E Test Result nirovins/fix_pf_recordings

  4 files    4 suites   4m 18s ⏱️
244 tests 204 ✅  40 💤 0 ❌
976 runs  816 ✅ 160 💤 0 ❌

Results for commit a75fe00.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jun 28, 2024

promptflow-evals test result

  9 files    9 suites   2h 42m 28s ⏱️
 54 tests  49 ✅  5 💤 0 ❌
486 runs  441 ✅ 45 💤 0 ❌

Results for commit a75fe00.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jun 28, 2024

SDK CLI Test Result nirovins/fix_pf_recordings

    4 files      4 suites   1h 8m 30s ⏱️
  786 tests   763 ✅ 23 💤 0 ❌
3 144 runs  3 052 ✅ 92 💤 0 ❌

Results for commit a75fe00.

♻️ This comment has been updated with latest results.

return value


def sanitize_pf_run_ids(value: str) -> str:
Copy link
Contributor

@zhengfeiwang zhengfeiwang Jul 3, 2024

Choose a reason for hiding this comment

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

just curious why we need such a utility here? is there any limitation that randstr cannot support the scenario?

in pf-azure tests, it works well so that we don't need to apply sanitization, so i'd like to know the limitation here.

# Sanitize telemetry event
if isinstance(body_dict, list) and "Microsoft.ApplicationInsights.Event" in body:
body_dict = SanitizedValues.FAKE_APP_INSIGHTS
# Sanitize the artifact paths
Copy link
Contributor

@zhengfeiwang zhengfeiwang Jul 3, 2024

Choose a reason for hiding this comment

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

code missing here, or reluctant comment line?

"parentRunUuid": null, "rootRunUuid": "a5390638-bc06-4129-b24f-2a96292f0df3",
"lastStartTimeUtc": "2024-06-18T18:21:00.126+00:00", "currentComputeTime":
"00:00:00", "computeDuration": null, "effectiveStartTimeUtc": "2024-06-18T18:21:00.126+00:00",
"token": "eyJhbGciOiJSUzI1NiIsImtpZCI6IkZBMTRBODAyQjhDRTQ0MjQxRjE0NkU1RDgyNUQwNzJDODE3NjM0ODQiLCJ0eXAiOiJKV1QifQ.eyJyb2xlIjoiQ29udHJpYnV0b3IiLCJzY29wZSI6Ii9zdWJzY3JpcHRpb25zL2IxNzI1M2ZhLWYzMjctNDJkNi05Njg2LWYzZTU1M2UyNDc2My9yZXNvdXJjZUdyb3Vwcy9uaXJvdmlucy1yZy1lYXN0dXMvcHJvdmlkZXJzL01pY3Jvc29mdC5NYWNoaW5lTGVhcm5pbmdTZXJ2aWNlcy93b3Jrc3BhY2VzL25pcm92aW5zLWFpLXByb2plY3QiLCJhY2NvdW50aWQiOiIwMDAwMDAwMC0wMDAwLTAwMDAtMDAwMC0wMDAwMDAwMDAwMDAiLCJ3b3Jrc3BhY2VJZCI6ImY2YTcyYTc0LWY1NWEtNGRiOS04NmNmLTBiNTc3ZTZkYWQzZCIsInByb2plY3RpZCI6IjAwMDAwMDAwLTAwMDAtMDAwMC0wMDAwLTAwMDAwMDAwMDAwMCIsImRpc2NvdmVyeSI6InVyaTovL2Rpc2NvdmVyeXVyaS8iLCJ0aWQiOiI3MmY5ODhiZi04NmYxLTQxYWYtOTFhYi0yZDdjZDAxMWRiNDciLCJvaWQiOiI3ODc1YmFhNi0wMjhkLTQ5YzgtYmQ0ZC1hNzcyYWE0ZGMwMTMiLCJwdWlkIjoiMTAwM0JGRkRBOEE0RDBFNyIsImlzcyI6ImF6dXJlbWwiLCJhcHBpZCI6Ik5pa29sYXkgUm92aW5za2l5IiwiZXhwIjoxNzIxMzUwNjYyLCJhdWQiOiJhenVyZW1sIn0.STZiicI55Jz8o0PSqzP3641BU7wgxpe5z68nnRlQbtE2UNEu_MawQl156nHHFe8On7h-_9T2aphGlou_iEO1dQfG9TkckNV86Hbl-qwKB_vDusdd9FEM6u87EiXZYJ6m9YOqQBTs0DSzYaEOn30mW0lRXvdvb-bZCKYs4ICSkR_JMEkvs8qdHAIWxe9puMC8KGKm_VHVP0zuklsFwOw6XJ05CZ06H8mf9cN948kPKgDBlTXN9TMzS02qoVHra_Vnyt84QRMVBCAfdrLghc4BrmEN75S0o9upc2VeTaY_8JhXo51UQP3qBpRohlNRU0vx1Bv_kytu8uNPuq5FiXuEWQ",
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it's better to sanitize this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants