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 perf-counter instead of time #643

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

ykeremy
Copy link
Contributor

@ykeremy ykeremy commented Jul 25, 2024

🚀 This description was created by Ellipsis for commit 27671a2

Summary:

Replaced time.time() with time.perf_counter() in llm_api_handler for more precise time measurements.

Key points:

  • Replaced time.time() with time.perf_counter() in skyvern/forge/sdk/api/llm/api_handler_factory.py.
  • Affects llm_api_handler function.
  • Improves precision of request and cancellation time measurements.

Generated with ❤️ by ellipsis.dev

@ykeremy ykeremy merged commit 20a611a into main Jul 25, 2024
2 checks passed
@ykeremy ykeremy deleted the ykeremy/use-perf-counter-instead-of-time branch July 25, 2024 06:47
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 27671a2 in 23.584344 seconds

More details
  • Looked at 23 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/sdk/api/llm/api_handler_factory.py:198
  • Draft comment:
    The change to time.perf_counter() is appropriate for performance measurement. However, ensure that all uses of these timestamps are consistent and correctly interpreted throughout the codebase. If these timestamps are used in contexts that expect real-time timestamps, this could lead to issues.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The PR changes the time measurement from time.time() to time.perf_counter() for better precision in measuring intervals. This is generally a good practice for performance measurement as time.perf_counter() provides a higher resolution timer which is more suitable for timing operations. However, the PR does not update the logging or usage of these timestamps, which could lead to confusion or incorrect calculations if the values are used elsewhere in a context that expects a real-time timestamp rather than a performance counter. It's important to ensure that all uses of these timestamps are consistent and correctly interpreted.

Workflow ID: wflow_080bmDXvYQDJUC8K


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

1 participant