feat(LAB-4614): return a clear error when isUsedForConsensus is refused - #2062
Draft
RuellePaul wants to merge 1 commit into
Draft
feat(LAB-4614): return a clear error when isUsedForConsensus is refused#2062RuellePaul wants to merge 1 commit into
RuellePaul wants to merge 1 commit into
Conversation
The backend rejects `isUsedForConsensus` in `updatePropertiesInAssets` on multi-review projects with an opaque `[unexpectedServiceError]` wrapped in a GraphQL transport error. It now emits a domain error tagged `[isUsedForConsensusDeprecated]`, which the SDK translates into a `DeprecatedArgumentError` pointing at `update_asset_consensus`. The argument is not rejected client-side: it still works on workflow v1 projects, where `update_asset_consensus` is not available. The field is still sent and the error is only reinterpreted when the backend refuses it. Also documents that `update_asset_consensus` is not compatible with workflow v1, and updates the consensus tutorial to show both methods.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
kili.update_properties_in_asset
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose: Translate the backend's new deprecation error into a typed SDK exception, so the user sees an actionable message instead of a
TransportQueryErrorwrapping a JS stack trace.Paired MR:
kili!14392. Ships in either order — against a pre-deploy backend the token is simply absent and the user sees today's error. Graceful degradation.Plan document: https://linear.app/kili-technology/document/lab-4614-aau-i-get-the-right-error-message-when-using-kiliupdate-ce9688058bc8
Approach: pass through, do NOT reject client-side
Five reasons, all verified rather than assumed:
project_idis optional onupdate_properties_in_assets— a client-side check would need up to two extra round trips on every call using the argument.asset_idscan span multiple projects, which may mix workflow v1 and v2. A single client-side verdict cannot express that; the backend evaluates per project.retry_all(...)whose final conjunct matches none of this message, so there is no retry andstop_after_delay(3*60)never engages. One round trip, immediate raise.src/kili/entrypoints/mutations/asset/helpers.pyis untouched — verified explicitly:git diff origin/main -- helpers.pyis empty."isUsedForConsensus": is_used_for_consensus_arrayis still at line 51 and still forwarded.An earlier draft of this change stripped that parameter and its mapping. That would have silently disabled consensus for every workflow-v1 user, with no error at all — the field is still valid and still honoured on v1. The regression guard is a test asserting
isUsedForConsensusis present in thedataArraysent on the v1 happy path.Acceptance criterion — verified end to end, not inferred
Ran the real path with the literal backend string. The user sees exactly:
Byte-identical to the AC.
__cause__rendersGraphQL error at index 0: [isUsedForConsensusDeprecated] …— the "at index 0" proves the test exercises the realmutate_from_paginated_callre-wrap rather than a synthetic error. The exception is aValueErrorsubclass, so existingexcept ValueErrorhandlers keep working.Contract compliance
Matches on the literal substring
[isUsedForConsensusDeprecated], scanning every element of the errors array — necessary becauseGraphQLError.__init__only ever readserror[0].extensions.codeis ignored entirely (it is the genericOPERATION_RESOLUTION_FAILURE). The prose after the token is not matched, so backend copy edits cannot break the SDK. Bothtrueandfalseare covered.Scoping note: the guard fires on
workflowVersion >= 2— v2 and v3 (v3 is "multi-step labeling", also multi-review). The acceptance message reads as an unconditional deprecation but is not one: the parameter still works on v1, andupdate_asset_consensusexplicitly rejects v1. Release notes should say "on multi-review projects (workflow v2+)".Documentation fixes
The backend confirmed new projects default to workflow v2 on every path the repo controls. So a reader following the published
set_up_workflowstutorial today creates a v2 project and hits this exact error. The executable cell is swapped toupdate_asset_consensusrather than merely annotated, while stating that the correct setter depends on the project's workflow version — an on-prem instance withFLAG_MULTI_STEP_REVIEW=falsestill gets v1. The.mdis regenerated by the repo hook, not hand-edited.update_asset_consensusandkili.assets.update_consensusnow document their workflow-v1 incompatibility, which neither mentioned before — a second documentation gap this ticket exposed.Post-review fixes (agent self-review)
Raises:section activatedmissing-raises-doc→ documentedMissingArgumentErrorandGraphQLErrortoo.presentation/client/asset.pywent 999 → 1002 lines, trippingtoo-many-lines. Confirmed a regression rather than pre-existing by running pylint against the pristineorigin/maincopy (10.00/10) → fixed with the established# pylint: disable=too-many-linesconvention, already used by 6 modules including siblingclient/label.py.Raises:line forMissingArgumentErrorwas factually wrong — written as "if both are provided", but it is also raised when neither is → corrected._has_error_keygenerator expression sat exactly on the 100-char boundary where the local ruff 0.15.14 and the repo-pinned 0.1.15 could disagree — CI would break whichever way it was written → rewritten as an explicit loop so no formatter has a choice.is_used_for_consensus_array is not Noneas well as the token — rejected because the backend only emits the token when the field is in the payload, so it guards an unreachable state.Test honesty: tests 2 and 3 pass against pre-change code by design — they are regression guards, not new-behaviour tests. Test 1 is the only one that fails without the fix, and it does fail without it.
Verification
Baseline captured from a pristine tree before the first edit: 748 passed / pyright 0 / pylint 10.00. After: 751 passed — exactly +3, no regressions. pylint 10.00 restored, pyright 0, ruff set-comparison shows zero new findings. Full pre-commit suite passed at commit time including the repo-pinned ruff 0.1.15.
tests/e2e/— needs a live Kili instance and API key. CI runs unit+integration only, which is exactly what was run. No e2e test referencesis_used_for_consensus_array, but that is not proof.ruff checkreports 4119 findings on the implementing host — purely the 0.15.14 vs pinned 0.1.15 version gap, present on pristineorigin/maintoo.🤖 Planned and implemented by Claude Code.