ENG 2439 add engine and resource to metrics and checks - #1049
Conversation
|
@kenxu95 what's the easiest way to test these changes if I don't have an existing engine set up? |
|
You can try rebasing on #1046 and running the periodic integration tests manually. Otherwise, I'd recommend setting up your own cluster - it's super easy with Terraform. |
kenxu95
left a comment
kenxu95
left a comment
There was a problem hiding this comment.
Cool! One small refactor request and we should be good to go
| return multiplier * int(memory_scalar_str) | ||
|
|
||
|
|
||
| def _update_operator_spec_with_engine_and_resource( |
There was a problem hiding this comment.
Can we split this into two methods, one for updating engine and one for resources.
| def _update_operator_spec_with_engine_and_resource( | ||
| spec: OperatorSpec, | ||
| engine: Optional[str] = None, | ||
| resources: Optional[Dict[str, Any]] = None, |
There was a problem hiding this comment.
I wonder if our resource keys should be enums?
| SystemMetric *system_metric.SystemMetric `json:"system_metric,omitempty"` | ||
|
|
||
| // This can currently only be set for function operators. | ||
| // This can currently only be set for operators that has function, |
There was a problem hiding this comment.
maybe "for function operators, including metrics and checks."
There was a problem hiding this comment.
We've been using this in @op so I'd rather keep the same way for now.
|
Quick question on this — is it possible to share the logic for these flags in some way such that we automatically inherit changes between |
|
@vsreekanti good point, it requires us to refactor function decorator to class decorator (and use the inheritance pattern) so I'm putting a linear task for this: |
…g-2439-fix-bug-where-metric-and-check-decorator
…g-2439-fix-bug-where-metric-and-check-decorator
|
Mypy failures are known to happen on other PRs, merging giving it's passing locally. |
4 participants
Describe your changes and why you are making these changes
This PR adds engine and resource configs for metrics and checks so that the behavior is consistent to
@op()decorators. We simply moved the relevant parts in@op()decorator to a helper function and applies it to bothcheck()andmetrics()implementations.Related issue number (if any)
ENG 2439
Loom demo (if any)
Testing WIP
Checklist before requesting a review
python3 scripts/run_linters.py -hfor usage).run_integration_test: Runs integration testsskip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)