2686 add checkpoint with repoid #800
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #800 +/- ##
==========================================
- Coverage 97.99% 97.99% -0.01%
==========================================
Files 446 446
Lines 36568 36630 +62
==========================================
+ Hits 35835 35895 +60
- Misses 733 735 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #800 +/- ##
==========================================
- Coverage 97.99% 97.99% -0.01%
==========================================
Files 446 446
Lines 36568 36630 +62
==========================================
+ Hits 35835 35895 +60
- Misses 733 735 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #800 +/- ##
==========================================
- Coverage 97.99% 97.99% -0.01%
==========================================
Files 446 446
Lines 36568 36630 +62
==========================================
+ Hits 35835 35895 +60
- Misses 733 735 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #800 +/- ##
==========================================
- Coverage 97.99% 97.99% -0.01%
==========================================
Files 446 446
Lines 36568 36630 +62
==========================================
+ Hits 35835 35895 +60
- Misses 733 735 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| def log_counters(obj: T) -> None: | ||
| metrics.incr(f"{klass.__name__}.events.{obj.name}") | ||
| PROMETHEUS_HANDLER.log_checkpoints(flow=klass.__name__, checkpoint=obj.name) | ||
| def log_counters(obj: T, context: CheckpointContext = None) -> None: |
There was a problem hiding this comment.
| def log_counters(obj: T, context: CheckpointContext = None) -> None: | |
| def log_counters(obj: T, context: CheckpointContext | None = None) -> None: |
the same pattern repeats a bunch of times below. If the default is None, then None has to appear in the type as well.
| ): | ||
| self.cls = cls | ||
| self.data = data if data else {} | ||
| self.kwargs_key = _kwargs_key(self.cls) | ||
| self.strict = strict | ||
| self.context = context if context else {} |
There was a problem hiding this comment.
This should just be context in this case, as defaulting to {} does not really make sense.
| @@ -67,28 +126,46 @@ class PrometheusCheckpointLoggerHandler: | |||
| methods in this class are mainly used by the CheckpointLogger class. | |||
| """ | |||
|
|
|||
| def log_begun(self, flow: str): | |||
| def log_begun(self, flow: str, repo_id: int = None): | |||
There was a problem hiding this comment.
| def log_begun(self, flow: str, repo_id: int = None): | |
| def log_begun(self, flow: str, repo_id: int | None = None): |
same here, the argument type needs to match up with the default value.
| kwargs: MutableMapping[str, Any], | ||
| strict: bool = False, | ||
| context: CheckpointContext = None, |
There was a problem hiding this comment.
for this particular case, you could just pick repo_id (aka repoid?) out of the kwargs.
There was a problem hiding this comment.
I don't think that would work off the bat as the repoid has been specified as a keyword parameter, thus not belonging to **kwargs when it's supplied to the checkpoints functionality, for instance here
Lines 300 to 301 in 5514b97
context to a) type the supplied params and b) serve as an object that has items additional to the checkpoint flow - I'm open to a different approach, I chose this one for readability + separation of concerns. (Although I will rename repo_id to repoid to be consistent with our model definition)
There was a problem hiding this comment.
Addressed your other suggestions, thanks 🙏
| cls: type[T], | ||
| kwargs: MutableMapping[str, Any], | ||
| strict: bool = False, | ||
| context: CheckpointContext | None = None, |
There was a problem hiding this comment.
if you take the TypedDict/self.data["context"] suggestion, you won't need to take this in from_kwargs. you will need to do a little extra work though
def from_kwargs(...):
# Copy so we don't modify the passed-in kwargs
data = kwargs.get(_kwargs_key(cls), {}).copy()
deserialized_data = {}
# Remove the "context" key. All remaining keys should be castable to checkpoints
deserialized_data["context"] = data.pop("context", CheckpointContext())
# for loop can remain the same
for checkpoints, timestamp in data.items():
...There was a problem hiding this comment.
So I had to play with this a bit to make it work. The kwargs.get(_kwargs_key(cls), {}).copy() is an empty object the first time a flow is ran, so instead I did
def from_kwargs(
cls: type[T],
kwargs: MutableMapping[str, Any],
strict: bool = False,
) -> CheckpointLogger[T]:
context = kwargs.pop("context", CheckpointContext())
data = kwargs.get(_kwargs_key(cls), {})
# kwargs has been deserialized into a Python dictionary, but our enum values
# are deserialized as simple strings. We need to ensure the strings are all
# proper enum values as best we can, and then downcast to enum instances.
deserialized_data = {}
deserialized_data["context"] = context
for checkpoint, timestamp in data.items():it could be prettier but that's what I could think, wdyt?
| context = kwargs.pop("context", CheckpointContext()) | ||
| data = kwargs.get(_kwargs_key(cls), {}) | ||
|
|
||
| # kwargs has been deserialized into a Python dictionary, but our enum values | ||
| # are deserialized as simple strings. We need to ensure the strings are all | ||
| # proper enum values as best we can, and then downcast to enum instances. | ||
| deserialized_data = {} | ||
| deserialized_data["context"] = context | ||
|
|
There was a problem hiding this comment.
there's a subtle bug here. you have to do data.pop("context") because otherwise the for loop later in this function that iterates over data.items() will explode when trying to cast "context" to an UploadFlow enum (or whichever flow). also i think kwargs.pop("context") will mutate the passed-in kwargs which might not be safe
taking a step back, i think we don't have a tidy way to manage this context. if we store it in self.data, then each flow's self.data has its own copy and that's redundant. if we put a single copy in various tasks' kwargs, we have to remember to manually pass it and make sure it's included in kwargs when we retry and yadda yadda. since we want to do similar things for log.info() calls (task_name, task_id) and SQL metrics (repo_id, owner_id, commit_sha), i wrote #810 which i think can neatly take care of it for all of them
We're extending our checkpoint metrics to add repo specific metrics. The original approach considered adding a repo_id label to the existing checkpoint logger, but this is an ask for a specific repository. This approach isn't necessarily scalable with many repositories as repo_id has high cardinality, which isn't very well suited for prometheus. The better approach would be to leverage sql metrics and extend their functionality, but this approach was chosen for times sake.
This PR
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.