fix: LEAP-634: Actualize label config validation #5313

Gondragos · 2024-01-19T17:54:42Z

Config validation didn't take into account many tags and their attributes. This PR might fix this.

It also fixes nested Choice validation.

It also will fix validation messages. (Usually there is no path at all and sometimes it doesn't tell about the problem itself)

PR fulfills these requirements

Tests for the changes have been added/updated
Docs have been added/updated
Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
Self-reviewed and ran all changes on a local instance

Explain the changes you've made

Changing exc.path into exc.absolute_path fixes displaying validation paths from Validation failed on : to Validation failed on View/Labels/Label: .
Changing

{
  "anyOf": [
    {
      "type": "array",
      "items": {"$ref": "#/definitions/ref1"}
    }, 
    {
      "$ref": "#/definitions/ref1"
    }
  ]
}

into

{
  "anyOf": [
    {
      "$ref": "#/definitions/ref1"
    },
    {
      "type": "array",
      "items": {"$ref": "#/definitions/ref1"}
    } 
  ]
}

fixes displaying cause of validation fails from OrderedDict() is not of type 'array' to something more meaningful like 'value' is a required property

The line

# config = _fix_choices(config)

was hidden under the comment due to the problem, which caused this workaround, was solved by json schema itself.

What alternative approaches were there?

Fixing displayed paths has leaded to the situation when

<View>
  <Text name="text" value="$text" />
  <Choices name="choice" toName="choice">
    <Choice />
    <Choice value="any" />
  </Choices>
</View>

shows View/Choices/Choice/0, but

<View>
  <Text name="text" value="$text" />
  <Choices name="choice" toName="choice">
    <Choice />
  </Choices>
</View>

shows View/Choices/Choice. And previously it was just Choice/0 in this case.

To make it more consistent we can use removed right now function _fix_choices but convert all tags into arrays and in the same time get rid of MaybeMultiple* rules in schema. But so far it doesn't look to me that it's necessary, 'cause doesn't really affects user's navigation.

This change affects (describe how if yes)

Performance
Security
UX

Does this PR introduce a breaking change?

Yes, and covered entirely by feature flag(s)
Yes, and covered partially by feature flag(s)
No
Not sure (briefly explain the situation below)

It depends on the validity of the validation rules.

What level of testing was included in the change?

e2e (codecept)
integration (cypress)
unit (jest)

Which logical domain(s) does this change affect?

Labeling Interface

netlify · 2024-01-19T17:54:48Z

✅ Deploy Preview for heartex-docs ready!

Name	Link
🔨 Latest commit	`4c0521b`
🔍 Latest deploy log	https://app.netlify.com/sites/heartex-docs/deploys/65b9c4c175233900080212d5
😎 Deploy Preview	https://deploy-preview-5313--heartex-docs.netlify.app/guide/billing
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2024-01-19T17:54:55Z

✅ Deploy Preview for label-studio-docs-new-theme ready!

Name	Link
🔨 Latest commit	`4c0521b`
🔍 Latest deploy log	https://app.netlify.com/sites/label-studio-docs-new-theme/deploys/65b9c4c13df3b30008064e90
😎 Deploy Preview	https://deploy-preview-5313--label-studio-docs-new-theme.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-01-22T16:01:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.79%. Comparing base (4e0f517) to head (4c0521b).
Report is 828 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5313      +/-   ##
===========================================
- Coverage    75.88%   75.79%   -0.10%     
===========================================
  Files          154      154              
  Lines        12931    12930       -1     
===========================================
- Hits          9813     9800      -13     
- Misses        3118     3130      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jombooth · 2024-01-31T01:06:32Z

label_studio/core/label_config.py

@@ -84,7 +84,7 @@ def parse_config_to_json(config_string):
    if xml is None:
        raise etree.ParseError('xml is empty or incorrect')
    config = xmljson.badgerfish.data(xml)
-    config = _fix_choices(config)
+    #  config = _fix_choices(config)


Is this intentionally left in? Why don't we want to _fix_choices anymore?

nevermind, I see this was mentioned in the description - my feeling is that if we really don't need this we can delete the line (?)

jombooth

I don't know that much about this label config validation code, and don't feel extremely confident approving without doing a sync review to understand its impact better. Is there anyone else more familiar with this who could take a look?

I will say we probably don't want to check in a commented-out function call; one way forward would be to remove the line and add a comment like

# previously we called `_fix_choices` here, but no longer need to because...

Gondragos · 2024-01-31T03:58:30Z

I will say we probably don't want to check in a commented-out function call; one way forward would be to remove the line and add a comment like
# previously we called `_fix_choices` here, but no longer need to because...

@jombooth Thanks for that! It looks much better.

jombooth

LGTM, if someone more knowledgeable can do a second look that'd be great but I think we should get this over to QA, and the changes look reasonable!

robot-ci-heartex · 2024-06-10T21:04:43Z

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

robot-ci-heartex · 2024-10-19T01:59:04Z

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

fix: LEAP-245: Actualize label config validation

ae4b50e

github-actions bot added the fix label Jan 19, 2024

Remove unnecessary validation

4fc649c

Gondragos force-pushed the fb-leap-245/config-validation branch from 464e4ff to 4fc649c Compare January 22, 2024 15:54

Gondragos added 9 commits January 24, 2024 01:37

Fix TimeSeries and Channel validation

a427e90

Merge branch 'develop' into fb-leap-245/config-validation

b4f14e8

Fix misprint

64e0162

Fix valueList validation

c0e46cc

Fix validation messages

c8205d5

Fix nested Choice validation

1f97ec1

Fix TimeSeries and Channel validation

c78470b

Fix linting problems

5c764f2

Fix tests

2520089

jombooth reviewed Jan 31, 2024

View reviewed changes

Gondragos added 2 commits January 31, 2024 03:43

Merge branch 'develop' into fb-leap-245/config-validation

643a454

Add comments for the future

4c0521b

Gondragos requested review from jombooth and removed request for jombooth January 31, 2024 03:59

jombooth approved these changes Jan 31, 2024

View reviewed changes

triklozoid approved these changes Jan 31, 2024

View reviewed changes

Gondragos changed the title ~~fix: LEAP-245: Actualize label config validation~~ fix: LEAP-634: Actualize label config validation Jan 31, 2024

robot-ci-heartex added the Stale label Jun 10, 2024

robot-ci-heartex removed the Stale label Jul 26, 2024

robot-ci-heartex added Stale and removed Stale labels Oct 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: LEAP-634: Actualize label config validation #5313

fix: LEAP-634: Actualize label config validation #5313

Gondragos commented Jan 19, 2024 •

edited

Loading

netlify bot commented Jan 19, 2024 •

edited

Loading

netlify bot commented Jan 19, 2024 •

edited

Loading

codecov bot commented Jan 22, 2024 •

edited

Loading

jombooth Jan 31, 2024

jombooth Jan 31, 2024

jombooth left a comment

Gondragos commented Jan 31, 2024 •

edited

Loading

jombooth left a comment

robot-ci-heartex commented Jun 10, 2024

robot-ci-heartex commented Oct 19, 2024

fix: LEAP-634: Actualize label config validation #5313

Are you sure you want to change the base?

fix: LEAP-634: Actualize label config validation #5313

Conversation

Gondragos commented Jan 19, 2024 • edited Loading

PR fulfills these requirements

Explain the changes you've made

What alternative approaches were there?

This change affects (describe how if yes)

Does this PR introduce a breaking change?

What level of testing was included in the change?

Which logical domain(s) does this change affect?

netlify bot commented Jan 19, 2024 • edited Loading

✅ Deploy Preview for heartex-docs ready!

netlify bot commented Jan 19, 2024 • edited Loading

✅ Deploy Preview for label-studio-docs-new-theme ready!

codecov bot commented Jan 22, 2024 • edited Loading

Codecov Report

jombooth Jan 31, 2024

Choose a reason for hiding this comment

jombooth Jan 31, 2024

Choose a reason for hiding this comment

jombooth left a comment

Choose a reason for hiding this comment

Gondragos commented Jan 31, 2024 • edited Loading

jombooth left a comment

Choose a reason for hiding this comment

robot-ci-heartex commented Jun 10, 2024

robot-ci-heartex commented Oct 19, 2024

Gondragos commented Jan 19, 2024 •

edited

Loading

netlify bot commented Jan 19, 2024 •

edited

Loading

netlify bot commented Jan 19, 2024 •

edited

Loading

codecov bot commented Jan 22, 2024 •

edited

Loading

Gondragos commented Jan 31, 2024 •

edited

Loading