Repository navigation
Conversation
059aecb to
a57e266
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
a57e266 to
49f2237
Compare
There was a problem hiding this comment.
An organization admin can review the limit in your organization's usage and billing settings.
Once the limit resets or is raised, someone with write access to this repository can comment @claude review on this pull request to trigger a review.
| and that a malformed source raises with its location.""" | ||
| """Check four properties of the ARFBench series connector. | ||
|
|
||
| A metric keeps every rollup per tag group. A question becomes a classification task over its options. The dataset |
There was a problem hiding this comment.
| A metric keeps every rollup per tag group. A question becomes a classification task over its options. The dataset | |
| Metrics contain every rollup per tag group. | |
| Questions are transformed into Classification Tasks with the Options as the Label-Set. The dataset... |
Still a little claudey
| @@ -0,0 +1 @@ | |||
| """ARFBench plots connector tests.""" | |||
There was a problem hiding this comment.
Does this actually plot anything? If so, should we put those plots into the test plan of this PR? If not, can we change the name?
There was a problem hiding this comment.
ah well I realised right after posting this that it is referring to the "charts" part. Hmmmmm...maybe we can then change arfbench to arfbench_timeseries and perhaps this to arfbench_charts. arfbench_plots makes me think you;ve plotted something here...
TimoStoff
left a comment
TimoStoff
left a comment
There was a problem hiding this comment.
LGTM! Some minor comments
| paths = release_paths(raw_refs, "questions", "charts") | ||
| series = context.parents[SERIES_PARENT] | ||
| dataset = TimeFDataset(metadata=self.metadata()) | ||
| dataset.import_from(series, tasks=[task.id for task in series.iter_tasks()]) |
There was a problem hiding this comment.
Do we need to import the series tasks here at all? As far as I can tell, there arent any Tasks that have both series and a chart together.
A chart-only dataset would be a plain convert connector with 750 tasks, and on the consumer side you could still pair the two datasets via question_index right? Anyways, ffeel free to ignore, just a passing thought....
| path.stem: dataset.add_record(record=_create_chart_record(path)) | ||
| for path in sorted(paths["charts"].glob("*.png")) | ||
| } | ||
| for question in read_questions(paths["questions"]): |
There was a problem hiding this comment.
The child takes every task of the parent and then reads the question table again here, butwe dont add any validation. So i suppose if the registry contains a parent that was built from other data, e.g old 1,500-task datadog/arfbench@1.0.0 from #615 we would get a bvroken dataset, but no indication of that. Maybe worth validating and failing. I have had weird shit like this happen to me actually, with all the arbitrary pins to particular TimeNet versions in the last days....
| label_set=LabelSet( | ||
| name=f"question_{self.index}", labels=tuple(Label(name=option) for option in self.answer_options) | ||
| ), |
There was a problem hiding this comment.
This makes one label set per question, so 750 sets.
I got Claude to check the pinned question table and only 278 option lists are distinct, and three of them cover 444 of the 750 questions (for example the yes/no list is used 111 times).
If we were to name the set from its content, (e.g short hash of the option tuple), the equal option lists coudl share sets
|
|
||
| def _create_chart_record(path: Path) -> Record: | ||
| """Create the Record of a chart, with the image as its one signal. | ||
| def release_paths(raw_refs: list[HubFileRef], *parts: str) -> dict[str, Path]: |
There was a problem hiding this comment.
I think this is generic enough that you can put it in BaseHuggingFaceConnector
| Returns: | ||
| The dataset: the series tasks first, then the chart tasks, paired by ``question_index``. | ||
| """ | ||
| self.check_refs(raw_refs) |
There was a problem hiding this comment.
From Claude:
This call is needed by hand because the base only wraps convert with check_refs, not compose. The next connector that implements only compose and forgets this line skips the check with no error. Can the base (#770) wrap compose too? Then this line can go.
| # ARFBench asks 750 multiple-choice questions about 142 metrics from 63 Datadog incidents. A metric | ||
| # is one monitored quantity, such as the request count of a service. Datadog splits a metric by | ||
| # tags. Thus a metric holds one series per tag group, for example one series per host. | ||
| # | ||
| # Datadog stores a metric at several rollups. A rollup is the metric on a fixed time grid, with one | ||
| # value per interval of 10, 60, 300, 1800, 3600, or 86400 seconds. The rollups of a metric are | ||
| # separate exports, not resamplings of one series. Their time ranges and tag groups differ. | ||
| # | ||
| # Each metric is one record, with one source per tag group and one signal per rollup. A consumer | ||
| # chooses the rollup. The paper and the charts use the finest rollup. The annotation | ||
| # `finest_interval_s` names this rollup. | ||
| # | ||
| # A rollup file lists the tag groups at the steps of its grid. Where the metric reported nothing | ||
| # for a group, the step holds a null or is not in the file. The dataset cuts each series to the | ||
| # window from the first value of its group to the last value. Thus a group with no value at a | ||
| # rollup has no series at that rollup. Inside the window, each such step is a missing timestep on | ||
| # the regular grid, with the reason `not_recorded` from the spec. Values are float64. The release | ||
| # names no units. The release stores naive epochs. The dataset reads them as UTC. The charts | ||
| # and the answer options use the same clock. |
There was a problem hiding this comment.
These lines, and the "keeps three items as released" list further down, are copies of the comment in the datadog/arfbench card. Maybe here we describe only the charts and the pairing, and reference to the parent card for the series?
| description: >- | ||
| 750 multiple-choice questions about anomalies in 142 metrics from 63 Datadog incidents. | ||
| The dataset includes every published metric rollup and the released charts. |
There was a problem hiding this comment.
this is still the description of the old combined dataset....
| return qa_csv | ||
|
|
||
|
|
||
| def facts(annotated) -> dict: |
There was a problem hiding this comment.
annotated has no type and the return type is a bare dict. Maybe SupportsAnnotate and dict[str, object]?
| # - 29 time options that are outside every rollup. | ||
| yaml_schema_version: 1 | ||
| dataset_id: datadog/arfbench | ||
| dataset_version: 1.0.0 |
There was a problem hiding this comment.
are we bumping versions, now that it is a public release?
| @@ -0,0 +1 @@ | |||
| """ARFBench plots connector tests.""" | |||
There was a problem hiding this comment.
ah well I realised right after posting this that it is referring to the "charts" part. Hmmmmm...maybe we can then change arfbench to arfbench_timeseries and perhaps this to arfbench_charts. arfbench_plots makes me think you;ve plotted something here...
Description The ARFBench dataset held the metric series and the released charts. The charts are image tensors, which only the Zarr backend stores, so the whole dataset used Zarr, and loading it downloaded every chart. A consumer of the series, such as a time-series language model, needs neither. `datadog/arfbench` now holds the metric records and one series task per question, with the default Parquet backend. Its requirements drop Pillow and Zarr. The new `datadog/arfbench-plots` connector subclasses it: it uses Zarr, also downloads the charts, and adds the chart records and one chart task per question after the series tasks. Both tasks of a question keep the question's row as `question_index`. Both datasets are version 1.0.0. * `arfbench/connector.py` reads the table and the series. The download patterns are a class attribute that the plots connector extends. * `arfbench_plots/connector.py` adds the charts on top of `convert`. * `arfbench/tests/release.py` writes the test release without Pillow, so both connectors' tests share it. Test plan - Command: `make test-connectors` steps for both connectors: pytest and ty in each connector's own environment - Coverage: the series tests from #615, a Parquet round trip that keeps the missing timesteps, and the plots tests for the chart records, the paired tasks, the Zarr round trip, and a pair without a combined chart - Command: `make check` - Coverage: formatting, lint, generated schemas, and types pass - Converting the pinned release gives 142 records, 38,032 series (3,502 with gaps), and 750 tasks for `datadog/arfbench`, and 556 records and 1,500 tasks for `datadog/arfbench-plots`. Writing the series dataset with Parquet and reading it back gives every value unchanged.
Description The review of #615 asked for three changes that this stack can make right away: * The requirements drop pyarrow, which the connector base installs, and ask for huggingface_hub>=1.1, as the other Hub connectors do. The plots connector keeps Pillow and Zarr, which the base does not install. * Both dataset cards name the v1 card schema. Test plan - Command: pytest and ty for both connectors in their own environments, as `make test-connectors` runs them - Coverage: 13 series tests and 4 plots tests pass; no code changes
…e the plots on the series Description The review of #615 asked for two more changes, and the plots dataset can now use TimeNet's own parent mechanism instead of subclassing: * Both connectors build on `BaseHuggingFaceConnector`, whose source parts can now be file patterns (#770). The series connector declares the question table and `arfbench-ts-data/*.parquet`; the plots connector the table and `arfbench-images/*.png`. * Every question asks to pick one of its released options, so each task is a classification task whose label set holds those options, named after the question's row. The series task and the chart task of a question share it. The `answer_options` annotation goes. * `datadog/arfbench-plots` declares `datadog/arfbench@1.0.0` as its parent `series`. Its `compose` imports the parent's tasks with the metric records they cite and adds the charts and the chart tasks. The imported series keep reading the parent's Parquet release, so the child stores only the charts, with Zarr. Test plan - Command: pytest and ty for both connectors in their own environments, as `make test-connectors` runs them - Coverage: 14 series tests (the classification tasks and their label sets, the Parquet round trip) and 4 plots tests (the parent declaration, the imported and added tasks, a child that stores only the charts and reads the series through its parent, a missing chart) - Command: `make check` - Coverage: formatting, lint, generated schemas, and types pass - Command: `timenet-build build datadog/arfbench-plots --build-parents` into a local registry - Coverage: `datadog/arfbench` holds 142 records, 38,032 series, and 750 classification tasks; `datadog/arfbench-plots` holds 556 records and 1,500 tasks over the same 750 label sets, names `datadog/arfbench@1.0.0` as its payload source, and stores the charts
…n simple English Description The docstrings, comments, and dataset-card comments that this PR adds now follow the rules of simplified technical English: one fact per sentence, sentences of 25 words or fewer, and no semicolons. No code changes. Test plan - Command: pytest for both connectors in their own environments - Coverage: 14 series tests and 4 plots tests pass - Command: `make check` - Coverage: formatting, lint, generated schemas, and types pass
49f2237 to
e081cd5
Compare
|
| Check | Status |
|---|---|
| Quick checks | ❌ Failed |
| Tests (Python 3.13) | ⏭️ Did not run |
| Tests (Python 3.11) | ⏭️ Did not run |
Full output is in the job logs. Reproduce locally:
make lint-fix # auto-fix
make check-ci # the hooks, plus ty on 3.13 and 3.11
make test-unit # the quick job's tests
make test # the full suiteUpdated for e081cd5 · run #8371
arfbench had duplicated signals in the records; once as TS and once as images (Zarr) which created also twice as many tasks. This PR splits it up into 2 datasets, a plain one with TS only, a second one that inherits from this and adds the images using Zarr.
Description
ARFBench held the metric series and the released charts in one dataset. The charts are image
tensors, and only the Zarr backend stores image tensors. Thus the whole dataset used Zarr. Every
load downloaded all charts. A consumer of the series, for example a time-series language model,
does not need the charts or Zarr. This PR splits the dataset in two. It also applies the review of
#615.
datadog/arfbenchholds the 142 metric records and one series task per question. It uses thedefault Parquet backend.
datadog/arfbench-plotsis a child ofdatadog/arfbench. Its card declares the parentseries.Its
composeimports the tasks of the parent, together with the metric records that these taskscite. Then it adds one record per chart and one chart task per question. The imported series
continue to read the Parquet release of the parent. Thus the child stores only the charts, with
Zarr. Both tasks of a question keep the row of the question as
question_index.The label set of each task holds these options. The name of the label set comes from the row of
the question. This PR removes the
answer_optionsannotation.BaseHuggingFaceConnectorwith file patterns (feat(connectors): let a Hub connector declare a source part by file pattern #770). The series connectordeclares the question table and
arfbench-ts-data/*.parquet. The plots connector declares thetable and
arfbench-images/*.png.huggingface_hub>=1.1. The plots connector keeps Pillow and Zarr. The base does not installthem. Both cards name the v1 card schema.
Test plan
make test-connectorsruns them
tasks and their label sets, the Parquet round trip with missing timesteps, and malformed sources.
The plots tests cover the parent declaration, the imported and added tasks, and a missing chart.
They also cover a child that stores only the charts and reads the series through its parent.
make checktimenet-build build datadog/arfbench-plots --build-parentsinto a local registrydatadog/arfbenchholds 142 records, 38,032 series (3,502 with gaps), and 750classification tasks.
datadog/arfbench-plotsholds 556 records and 1,500 tasks. It namesdatadog/arfbench@1.0.0as its payload source. It stores only the charts.