Seaprate run_level information from task-level in the JSONL#118
Seaprate run_level information from task-level in the JSONL#118areebtariq-skylabs wants to merge 1 commit intomainfrom
Conversation
…sk level information
|
Right now I just have modified the JSONL Output schema , if it's look okay I can move towards updating the Backend ingestion logic. |
areebtariq-skylabs
requested review from
gmalecha-at-skylabs and
jhaag-skylabs-ai
CI summary (Details)Active Repos
|
| Repo | Job Branch | Job Commit |
|---|---|---|
| ./ | main | f1faa4e |
| fmdeps/BRiCk/ | main | b62cb51 |
| fmdeps/auto/ | main | 2a53fc0 |
| fmdeps/auto-docs/ | main | b22de96 |
| bluerock/NOVA/ | skylabs-proof | 220d4a8 |
| bluerock/bhv/ | skylabs-main | 20ba397 |
| fmdeps/brick-libcpp/ | main | 204cf18 |
| fmdeps/ci/ | main | 68178de |
| vendored/elpi/ | skylabs-master | aa4475f |
| fmdeps/fm-ci/ | main | cfedfa4 |
| fmdeps/fm-tools/ | main | 6e85551 |
| psi/protos/ | main | 8fe3e7c |
| psi/backend/ | main | bcf0062 |
| psi/ide/ | main | 6b596cf |
| psi/data/ | main | d7b9b68 |
| vendored/rocq/ | skylabs-master | 6d192b5 |
| vendored/rocq-elpi/ | skylabs-master | e7c8227 |
| vendored/rocq-equations/ | skylabs-main | 737fdf9 |
| vendored/rocq-ext-lib/ | skylabs-master | 8172052 |
| vendored/rocq-iris/ | skylabs-master | 51c753a |
| vendored/rocq-lsp/ | skylabs-main | a8b7272 |
| vendored/rocq-stdlib/ | skylabs-master | 10bd9d7 |
| vendored/rocq-stdpp/ | skylabs-master | 8307c10 |
| fmdeps/skylabs-fm/ | main | 9de54e4 |
| vendored/vsrocq/ | skylabs-main | 39c9c5b |
Performance
| Relative | Master | MR | Change | Filename |
|---|---|---|---|---|
| -0.00% | 121503.0 | 121503.0 | -0.0 | total |
| -0.00% | 24210.1 | 24210.1 | -0.0 | ├ translation units |
| +0.00% | 97292.9 | 97292.9 | +0.0 | └ proofs and tests |
Full Results
| Relative | Master | MR | Change | Filename |
|---|---|---|---|---|
| -0.00% | 121503.0 | 121503.0 | -0.0 | total |
| -0.00% | 24210.1 | 24210.1 | -0.0 | ├ translation units |
| +0.00% | 97292.9 | 97292.9 | +0.0 | └ proofs and tests |
jhaag-skylabs-ai
requested review from
LennartATSkylabsAI and
rlepigre-skylabs-ai
jhaag-skylabs-ai
left a comment
jhaag-skylabs-ai
left a comment
There was a problem hiding this comment.
I added Rodolphe+Lennart as reviewers as well. Fwiw, I like the proposed changes; deduplicating verbose data is always a good idea in my book :~)
| agent_cls <doc text="Agent class provenance."> : agent_class_info; | ||
| agent <doc text="Agent instance provenance."> : agent_info; |
There was a problem hiding this comment.
I think we will always deploy a single agent class at a time over a batch of tasks, so putting the agent_class_info in the header makes sense.
While we currently construct agent instances in the same way for each task, that might not always be the case. For the sake of forwards compatibility I propose:
- in
run_taskwe addagent_infointo a (shared/threadsafe) map that uses the Agent instance checksum as a key - we include a footer with agent-instance specific data
which should still ensure that we deduplicate this verbose data in the current/common case of having agent instances with identical provenance.
Note: we should ensure that the (potentially partial) footer is always written into the output task file, even if some run_tasks raise errors; we may not want to add the agent_info into the map until after run returns without error.
| for reproducibility/comparison purposes | ||
| *) | ||
| agent_cls_checksum <doc text="Pseudo-unique checksum of the Agent class that attempted the task."> : string; | ||
| agent_checksum <doc text="Pseudo-unique checksum of the Agent instance that attempted the task."> : string; |
There was a problem hiding this comment.
cf. comment above regarding run_header; if we adopt my suggestion, we should retain the agent_checksum here for correlation purposes.
| dataset_id = None | ||
| if getattr(arguments, "task_file", None) is not None: | ||
| project_name = getattr(project, "name", "").strip() | ||
| if project_name: | ||
| dataset_id = project_name | ||
| if not dataset_id: | ||
| dataset_id = os.getenv("DATASET_NAME") or "default" |
There was a problem hiding this comment.
cc/ @rlepigre-skylabs-ai @LennartATSkylabsAI
I propose:
- we use
Project.get_id()as a stable interface for project identity instead of directly accessingproject.name - we adopt the invariant that
project.get_id()must yield a non-empty value -- eagerly porting existingtask.yamlfile as necessary, and failing fast with an explicit error instead of using"default"if this invariant is violated- Note: an invariant is something that is required for the code to behave correctly, and invariant violations are unremediable at runtime. This is different from
Exceptions and other forms of "soft" errors which callers may be able to (attempt to) remediate at runtime, e.g. by retrying a network call that timed out.
- Note: an invariant is something that is required for the code to behave correctly, and invariant violations are unremediable at runtime. This is different from
- we rename
dataset_idtoproject_idto match the semantic change in this PR - re
dataset_id: if we care to "name" a particular set of tasks in addition to tracking theproject_id, we do so using tags -- which already support effective filtering in the dashboard- Note: if we find it too annoying to supply these ENV variables on the command line, we can probably add extra
metadata(i.e."tags") to theTaskFile
- Note: if we find it too annoying to supply these ENV variables on the command line, we can probably add extra
cc/ @ehtesham-zahoor @gmalecha-at-skylabs
I realize that adopting my suggestions will require DB/schema changes; I think it's prudent to eagerly make these changes now since we're already planning to archive/refresh the data in the dashboard (maybe this week, otherwise next week; cf. https://github.com/SkyLabsAI/psi-verifier/issues/1001)
|
@areebtariq-skylabs should this PR be revived? If not, should any parts of it be cannibalized for other PRs/issues? |
|
Based on discussion w/Areeb, we're going to close this for now. We're now relying more on telemetry, and we can always change the JSONL in the future if necessary. |
2 participants
Currently, the JSONL schema treats every item as a task_result. This forces us to repeat run-level metadata (like run_id, dataset_id, and agent_provenance) within every single task entry, often requiring prefixes like TASKS_ to distinguish between scopes. ( Like in the TAGS )
This PR introduces a dedicated run item type at the start of the JSONL file. By decoupling run-level information from individual tasks.
we can also store provenance data once at the beginning of the file and read it directly, removing our dependency on TEMPO for t data ingestion endpoint. cc @jhaag-skylabs-ai