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

feat: allow setting compute multiplier on functions #2912

Closed
wants to merge 1 commit into from

Conversation

laktek
Copy link
Contributor

@laktek laktek commented Nov 25, 2024

What kind of change does this PR introduce?

Introduce compute multiplier setting on Edge Functions.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 12002617002

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 59.524%

Files with Coverage Reduction New Missed Lines %
internal/gen/keys/keys.go 5 12.9%
Totals Coverage Status
Change from base Build 12002049349: -0.03%
Covered Lines: 6381
Relevant Lines: 10720

💛 - Coveralls

@@ -156,7 +156,8 @@ Deno.serve({
console.error(`serving the request with ${servicePath}`);

// Ref: https://supabase.com/docs/guides/functions/limits
const memoryLimitMb = 256;
const computeMultiplier = Math.max(Math.min(parseFloat(functionsConfig[functionName].computeMultiplier ?? '1'), 4), 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const computeMultiplier = Math.max(Math.min(parseFloat(functionsConfig[functionName].computeMultiplier ?? '1'), 4), 1)
const computeMultiplier = Math.max(Math.min(functionsConfig[functionName].computeMultiplier ?? 1, 4), 1)

No need to parse from string because serialised json already sets this field as number.

Should also update the type definition for functionConfig https://github.com/supabase/cli/pull/2912/files#diff-44113b100b5a3840b42b5d08768b70a8f82055f9a7d11b37f31e2ea72d51e50dR52

computeMultiplier?: number

Optionally, consider emitting a log message when this value is clamped so users are aware of this limit when serving locally, ie. 1 <= x <=4. Otherwise the only way to know is via the api error when deploying.

VerifyJwt: function.VerifyJWT,
ImportMapPath: toFileURL(function.ImportMap),
EntrypointPath: toFileURL(function.Entrypoint),
ComputeMultiplier: computeMultiplier,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ComputeMultiplier: computeMultiplier,
ComputeMultiplier: function.ComputeMultiplier,

VerifyJwt: function.VerifyJWT,
ImportMapPath: toFileURL(function.ImportMap),
EntrypointPath: toFileURL(function.Entrypoint),
ComputeMultiplier: computeMultiplier,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ComputeMultiplier: computeMultiplier,
ComputeMultiplier: function.ComputeMultiplier,

@@ -44,6 +44,7 @@ func (s *EdgeRuntimeAPI) UpsertFunctions(ctx context.Context, functionConfig con
continue
}
}
computeMultiplier := function.ComputeMultiplier
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
computeMultiplier := function.ComputeMultiplier

nitpick for consistency with how other configs are accessed

@laktek
Copy link
Contributor Author

laktek commented Nov 27, 2024

I'm closing this for now until we figure out some internal decisions.

@laktek laktek closed this Nov 27, 2024
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