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

Remove client-side check for minimum CPU core request #2041

Merged
merged 7 commits into from
Aug 5, 2024

Conversation

devennavani
Copy link
Contributor

@devennavani devennavani commented Jul 22, 2024

Describe your changes

Remove client check for CPU core request being at least 0.1. Let's let the server enforce a minimum CPU core request.

Changelog

  • Removed client check for CPU core request being at least 0.1, deferring to server-side enforcement.

@devennavani devennavani changed the title Update minimum CPU core request to 0.125 Remove client-side check for minimum CPU core request Jul 23, 2024
@devennavani devennavani marked this pull request as ready for review July 23, 2024 14:31
@devennavani devennavani requested a review from ekzhang July 23, 2024 14:39
Copy link
Member

@ekzhang ekzhang left a comment

Choose a reason for hiding this comment

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

As discussed on Slack briefly with @mwaskom I think this is fine! We have a more accurate check on the server-side, and the client-side check could cause version skew over time if we kept it in.

@mwaskom
Copy link
Contributor

mwaskom commented Jul 23, 2024

Is one downside here that the error becomes dissociated from the code with the incorrect value? (Since the server-side error will come out of FunctionCreate, deep within the create_all_objects / Resolver stack?) Could imagine that being an ergonomic regression for apps with a lot of functions.

@devennavani devennavani merged commit ddcfb4f into main Aug 5, 2024
22 checks passed
@devennavani devennavani deleted the deven/update_minimum_cpu_core_request branch August 5, 2024 19:07
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