Fix CVAT text attributes coerced to int on annotation import#7252
Fix CVAT text attributes coerced to int on annotation import#7252joaquinhuigomez wants to merge 4 commits intovoxel51:developfrom
Conversation
_parse_value() aggressively coerces string values to numeric types. When a CVAT text attribute contains all-digit strings like "0001", it gets cast to int(1), losing leading zeros and semantic meaning. Added an attr_type parameter to _parse_value() so callers can indicate the CVAT attribute type. When the source type is "text", numeric coercion is skipped and the original string is preserved. Threaded the attribute type information from the CVAT task labels through the annotation parsing pipeline via attr_type_map.
WalkthroughThe PR propagates CVAT attribute input types through the annotation download pipeline and adds a Sequence Diagram(s)sequenceDiagram
participant User
participant FiftyOneDownloader as "FiftyOne\nDownloader"
participant CVATAPI as "CVAT\nAPI"
participant Parser as "CVAT\nParser"
participant ShapeTag as "CVATShape / CVATTag"
participant Dataset
User->>FiftyOneDownloader: request download_annotations(task_id, coerce_text_attrs?)
FiftyOneDownloader->>CVATAPI: fetch task, annotations, attribute schema
CVATAPI-->>FiftyOneDownloader: annotations + attr_type_map
alt coerce_text_attrs == true
FiftyOneDownloader->>Parser: _parse_shapes_tags(annotations, attr_type_map=None)
else
FiftyOneDownloader->>Parser: _parse_shapes_tags(annotations, attr_type_map)
end
Parser->>ShapeTag: instantiate CVATShape/CVATTag with attr_type_map
ShapeTag-->>Parser: _parse_value(value, attr_type) — preserve text when attr_type=="text"
Parser->>Dataset: create labels with parsed attributes
Dataset-->>User: dataset updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
fiftyone/utils/cvat.py (2)
7684-7704:⚠️ Potential issue | 🟠 MajorReturn raw values for CVAT
textattributes.This only skips numeric coercion. Text attrs still fall through to the boolean/
Nonenormalization below, so values like"false","None", and""are still rewritten on import. Return early whenattr_type == "text"so the original CVAT string survives untouched.🔧 Proposed fix
def _parse_value(value, attr_type=None): - if attr_type != "text": - try: - return int(value) - except: - pass - - try: - return float(value) - except: - pass + if attr_type == "text": + return value + + try: + return int(value) + except: + pass + + try: + return float(value) + except: + pass if etau.is_str(value): if value in ("True", "true"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiftyone/utils/cvat.py` around lines 7684 - 7704, The _parse_value function currently skips numeric coercion for attr_type == "text" but still normalizes booleans and None-like strings; modify _parse_value to return the original value immediately when attr_type == "text" so text attributes are not rewritten (i.e., add an early return for attr_type == "text" that returns the raw value) and keep the existing numeric and boolean/None coercion for non-text types.
6043-6073:⚠️ Potential issue | 🟠 MajorHonor
input_typein the scalar-tag branch.Now that
attr_type_mapis threaded into_parse_annotation(), the scalar path should use it too. Line 6049 still calls_parse_value()without the attribute's CVATinput_type, soStringFieldscalars stored as text attrs will still coerce"0001"to1; the first such value can also pinprev_typetointand cause later non-numeric strings in the same field to be dropped.🔧 Proposed fix
if assigned_scalar_attrs: num_attrs = len(anno["attributes"]) attr_ind = 0 + _attr_types = ( + attr_type_map.get(anno["label_id"], {}) + if attr_type_map + else {} + ) while label is None and attr_ind < num_attrs: + attr = anno["attributes"][attr_ind] label = _parse_value( - anno["attributes"][attr_ind]["value"] + attr["value"], + attr_type=_attr_types.get(attr["spec_id"]), ) attr_ind += 1 if label is not None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiftyone/utils/cvat.py` around lines 6043 - 6073, The scalar branch currently calls _parse_value(anno["attributes"][attr_ind]["value"]) without telling it the CVAT input type, so pass the attribute's input_type from attr_type_map when calling _parse_value (use the attribute id from anno["attributes"][attr_ind] to look up attr_type_map and call _parse_value(value, input_type=that_input_type) or fallback to None if missing); update the call inside the assigned_scalar_attrs loop that references _parse_value, and keep the existing prev_type/type-checking logic unchanged so parsing honors CVAT StringField vs numeric input types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@fiftyone/utils/cvat.py`:
- Around line 7684-7704: The _parse_value function currently skips numeric
coercion for attr_type == "text" but still normalizes booleans and None-like
strings; modify _parse_value to return the original value immediately when
attr_type == "text" so text attributes are not rewritten (i.e., add an early
return for attr_type == "text" that returns the raw value) and keep the existing
numeric and boolean/None coercion for non-text types.
- Around line 6043-6073: The scalar branch currently calls
_parse_value(anno["attributes"][attr_ind]["value"]) without telling it the CVAT
input type, so pass the attribute's input_type from attr_type_map when calling
_parse_value (use the attribute id from anno["attributes"][attr_ind] to look up
attr_type_map and call _parse_value(value, input_type=that_input_type) or
fallback to None if missing); update the call inside the assigned_scalar_attrs
loop that references _parse_value, and keep the existing prev_type/type-checking
logic unchanged so parsing honors CVAT StringField vs numeric input types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 13514b55-8188-405a-9f6b-f3513727fa5e
📒 Files selected for processing (1)
fiftyone/utils/cvat.py
Early return str(value) for text-type attributes in _parse_value to prevent numeric coercion of text values like '0001'. Thread attr_type through the scalar-tag branch so tag annotations also benefit from type-aware parsing, matching the existing pattern used in the shapes branch.
|
Addressed both CodeRabbit findings:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fiftyone/utils/cvat.py (1)
4893-4911:⚠️ Potential issue | 🟠 MajorMirror
input_typepropagation in the XML importers.This only populates type metadata for REST downloads.
CVATTaskLabels.from_labels_dict(),CVATImageAnno._parse_anno_dict(), andCVATVideoAnno._parse_anno_dict()still drop XMLinput_typeand call_parse_value()without it, soload_cvat_image_annotations()/load_cvat_video_annotations()will still coerce"0001"to1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiftyone/utils/cvat.py` around lines 4893 - 4911, The XML import path doesn't propagate per-attribute input_type, causing XML imports to lose type metadata and coerce string numeric values incorrectly; update CVATTaskLabels.from_labels_dict, CVATImageAnno._parse_anno_dict, and CVATVideoAnno._parse_anno_dict to extract attribute "input_type" from the XML labels (mirror the REST logic used in _get_attr_class_maps to build attr_type_map) and ensure every call that parses attribute values passes that input_type into _parse_value (so load_cvat_image_annotations/load_cvat_video_annotations receive and use the same attr_type_map).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fiftyone/utils/cvat.py`:
- Around line 6046-6060: The code currently treats an empty string value for
text scalar attributes as a valid parsed label, turning an unset scalar into ""
on round-trip; in the loop in fiftyone.utils.cvat where _attr_types is computed
and label is set via _parse_value, skip attributes whose attr_type == "text" and
whose raw attr["value"] is "" (i.e., continue to next attr instead of calling
_parse_value), so empty text scalars remain unset; refer to the
variables/functions _attr_types, attr_type_map, attr (attr["value"],
attr["spec_id"]), attr_type, and _parse_value to locate and implement this
conditional skip.
---
Outside diff comments:
In `@fiftyone/utils/cvat.py`:
- Around line 4893-4911: The XML import path doesn't propagate per-attribute
input_type, causing XML imports to lose type metadata and coerce string numeric
values incorrectly; update CVATTaskLabels.from_labels_dict,
CVATImageAnno._parse_anno_dict, and CVATVideoAnno._parse_anno_dict to extract
attribute "input_type" from the XML labels (mirror the REST logic used in
_get_attr_class_maps to build attr_type_map) and ensure every call that parses
attribute values passes that input_type into _parse_value (so
load_cvat_image_annotations/load_cvat_video_annotations receive and use the same
attr_type_map).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1e9479de-7e85-481e-8605-23ca2f52af20
📒 Files selected for processing (1)
fiftyone/utils/cvat.py
Empty string values for text-type attributes should return None (matching the existing behavior for non-text types at line 7714) so that unset CVAT text scalars don't round-trip as "".
|
Fixed the empty-string text scalar issue — Regarding the XML importer path ( |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fiftyone/utils/cvat.py (1)
4893-4911:⚠️ Potential issue | 🟠 MajorKeep
_get_attr_class_maps()backward compatible or update the remaining callers.Line 4911 changes this helper to return three values, but
tests/intensive/cvat_tests.pystill unpacks two. That path now fails withValueError: too many values to unpackbefore the new parsing logic runs.Minimal caller update outside this hunk
- attr_id_map, class_id_map = api._get_attr_class_maps(task_id) + attr_id_map, _, class_id_map = api._get_attr_class_maps(task_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiftyone/utils/cvat.py` around lines 4893 - 4911, The helper _get_attr_class_maps was changed to return three values (attr_id_map, attr_type_map, class_map_rev) which breaks existing callers that expect two; revert _get_attr_class_maps to return the original two-tuple (attr_id_map, attr_type_map) to preserve backward compatibility and move the class_map_rev construction into a new helper (e.g. _build_class_map_rev or _get_class_map_rev) that returns class_map_rev when callers need it; update any new code paths to call the new helper explicitly and leave existing callers (e.g. tests/intensive/cvat_tests.py) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fiftyone/utils/cvat.py`:
- Around line 4722-4724: XML import paths call _parse_value without type
metadata causing digit-only text attributes to be coerced; in
load_cvat_image_annotations and load_cvat_video_annotations, fetch the attribute
maps via self._get_attr_class_maps(task_id) (to obtain attr_type_map) and pass
that attr_type_map into calls to _parse_value (instead of calling
_parse_value(value) with no type info). Update the places that currently call
_parse_value without type metadata to use _parse_value(value, attr_type_map,
attr_id_map) (or the existing _parse_value signature that accepts type maps) so
parsing is type-aware; also apply the same change to the other XML import call
sites that use _parse_value without attr_type_map (references: functions
_parse_value, load_cvat_image_annotations, load_cvat_video_annotations, and
_get_attr_class_maps).
---
Outside diff comments:
In `@fiftyone/utils/cvat.py`:
- Around line 4893-4911: The helper _get_attr_class_maps was changed to return
three values (attr_id_map, attr_type_map, class_map_rev) which breaks existing
callers that expect two; revert _get_attr_class_maps to return the original
two-tuple (attr_id_map, attr_type_map) to preserve backward compatibility and
move the class_map_rev construction into a new helper (e.g. _build_class_map_rev
or _get_class_map_rev) that returns class_map_rev when callers need it; update
any new code paths to call the new helper explicitly and leave existing callers
(e.g. tests/intensive/cvat_tests.py) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c984dbc9-fc22-47fc-a806-7d396da77607
📒 Files selected for processing (1)
fiftyone/utils/cvat.py
| attr_id_map, attr_type_map, _class_map_rev = ( | ||
| self._get_attr_class_maps(task_id) | ||
| ) |
There was a problem hiding this comment.
Apply the same type-aware parsing to XML imports.
This fix only works for the REST download path, where attr_type is available from task metadata. Line 1702 and Line 2436 still call _parse_value() without type metadata, so load_cvat_image_annotations() and load_cvat_video_annotations() will still coerce digit-only text attrs from XML imports.
Also applies to: 7693-7695
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fiftyone/utils/cvat.py` around lines 4722 - 4724, XML import paths call
_parse_value without type metadata causing digit-only text attributes to be
coerced; in load_cvat_image_annotations and load_cvat_video_annotations, fetch
the attribute maps via self._get_attr_class_maps(task_id) (to obtain
attr_type_map) and pass that attr_type_map into calls to _parse_value (instead
of calling _parse_value(value) with no type info). Update the places that
currently call _parse_value without type metadata to use _parse_value(value,
attr_type_map, attr_id_map) (or the existing _parse_value signature that accepts
type maps) so parsing is type-aware; also apply the same change to the other XML
import call sites that use _parse_value without attr_type_map (references:
functions _parse_value, load_cvat_image_annotations,
load_cvat_video_annotations, and _get_attr_class_maps).
There was a problem hiding this comment.
Great work identifying and fixing this issue. The root cause analysis is spot on, and the fix in _parse_value() itself is reasonable.
I had one design concern I wanted to raise: the attr_type_map is currently threaded as a parameter through
download_annotations → _parse_shapes_tags → _parse_annotation → CVATShape/CVATTag → CVATLabel.__init__ → _parse_value
which adds up to about 20 places it's needed in the code, spanning 8 functions. Most of these are pure pass-thru _parse_shapes_tags and _parse_annotation don't use the map directly, they just relay it.
Since _parse_shapes_tags, _parse_annotation, and download_annotations are all methods on CVATAnnotationAPI, I think it might be simpler to store the map as instance state (self._attr_type_map) on the API object when it's built per-task in download_annotations. That way, _parse_annotation can read self._attr_type_map directly and only pass it to the CVATShape, CVATTag, and CVATLabel constructors that actually need it. This would eliminate the parameter from the signatures of _parse_shapes_tags and _parse_annotation entirely, cutting down the number of areas impacted by the proposed changes.
I also had a concern about backward compatibility. There may be users who have pipelines where text attributes containing text values like "42" or "true" are intentionally coerced to int or bool values by the current behavior (see Hyrum's Law). Making text preservation automatic would silently change their results.
One way I can think to handle this would be to gate the behavior behind an opt-in config parameter on CVATBackendConfig, something like preserve_text_values=Falsedownload_annotations would populate self._attr_type_map from the CVAT task labels; when disabled (the default), self._attr_type_map stays None and existing coercion behavior is preserved. The Config base class handles serialization automatically for public attributes, so it would round-trip through annotate() and load_annotations() without any additional plumbing. This would need to be updated in the documentation as well, but would permit users to opt in with:
dataset.annotate(anno_key, ..., preserve_text_values=True) # see preferred argument name belowOne final note regarding the changes on this PR. Including unit tests to cover the new, and old, behaviors will be immensely helpful. Adding or updating tests will ensure that changes, if merged, will have safeguards to ensure the new behavior going forward.
To summarize, the two suggestions are:
- Store
attr_type_maponCVATAnnotationAPIto cut the intermediate pass-through plumbing - Default to existing behavior, let users enable text preservation explicitly, likely needs documentation updates for CVAT
- Include unit tests to cover all proposed changes, (for either the current proposed changes or based on my recommendations)
Happy to discuss if I'm missing something about why the current approach is preferable. I appreciate the thorough work tracing the type info through the pipeline. Ultimately merging the changes is not up to me, but I'm providing feedback with what I think could give the best odds of the changes being accepted.
[edit]
Important
Using opt-in arg name coerce_text_attrs is preferred over the suggested preserve_text_values. The design eeps the same behavior, coerce_text_attrs=False as the default which uses the same behavior as today (no coercing), coerce_text_attrs=True to enforce coercing CVAT #text attributes as string values.
|
@Burhan-Q FYI I added some thoughts to the motivating issue here. TLDR, I think this PR makes sense. On nomenclature, I have a slight preference for On these points:
my personal opinion is:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7252 +/- ##
===========================================
- Coverage 99.10% 98.90% -0.21%
===========================================
Files 85 102 +17
Lines 23353 29472 +6119
===========================================
+ Hits 23143 29148 +6005
- Misses 210 324 +114
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I agree with argument name On the point of adding @joaquinhuigomez please make the appropriate updates when you're able to. If you need some time, that's understandable. Please leave a comment or react with 👍 on this comment to acknowledge you're able to make updates soon, otherwise I'll assume you're busy if there's no response after a few days. You could also ping me on the |
|
On it — will update with coerce_text_attrs=False as default. |
|
Added |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fiftyone/utils/cvat.py`:
- Around line 3361-3367: The public load path doesn't expose the
coerce_text_attrs flag: update fiftyone.utils.annotations.load_annotations() to
accept a coerce_text_attrs parameter (default False) and thread it through when
calling results.backend.download_annotations(results,
coerce_text_attrs=coerce_text_attrs); also update the abstract method signature
AnnotationBackend.download_annotations(self, results, coerce_text_attrs=False)
and concrete implementations (e.g., CVATBackend.download_annotations /
fiftyone/utils/cvat.py::download_annotations) to accept and pass the flag;
alternatively persist the choice on the results/run config and read it inside
results.backend.download_annotations if you prefer configuration-based
propagation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e33d4e3c-78a2-47e2-ae7a-6aeda08ed0fc
📒 Files selected for processing (1)
fiftyone/utils/cvat.py
| def download_annotations(self, results, coerce_text_attrs=False): | ||
| api = self.connect_to_api() | ||
|
|
||
| logger.info("Downloading labels from CVAT...") | ||
| annotations = api.download_annotations(results) | ||
| annotations = api.download_annotations( | ||
| results, coerce_text_attrs=coerce_text_attrs | ||
| ) |
There was a problem hiding this comment.
Expose coerce_text_attrs on the public load path.
fiftyone.utils.annotations.load_annotations() still calls results.backend.download_annotations(results) with no way to pass this flag, and the abstract AnnotationBackend.download_annotations() signature still omits it. As written, the legacy opt-in is unreachable from the standard API even though the backend/API now expose it. Please thread the option through the public helper or persist it on the run config/results.
Also applies to: 4649-4657
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 3361-3361: Boolean default positional argument in function definition
(FBT002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fiftyone/utils/cvat.py` around lines 3361 - 3367, The public load path
doesn't expose the coerce_text_attrs flag: update
fiftyone.utils.annotations.load_annotations() to accept a coerce_text_attrs
parameter (default False) and thread it through when calling
results.backend.download_annotations(results,
coerce_text_attrs=coerce_text_attrs); also update the abstract method signature
AnnotationBackend.download_annotations(self, results, coerce_text_attrs=False)
and concrete implementations (e.g., CVATBackend.download_annotations /
fiftyone/utils/cvat.py::download_annotations) to accept and pass the flag;
alternatively persist the choice on the results/run config and read it inside
results.backend.download_annotations if you prefer configuration-based
propagation.
3 participants
Fixes #7221
_parse_value()aggressively coerces string values to numeric types. When a CVAT text attribute contains all-digit strings like"0001", it gets cast toint(1), losing leading zeros and semantic meaning.Added an
attr_typeparameter to_parse_value()so callers can indicate the CVAT attribute type. When the source type is"text", numeric coercion is skipped and the original string is preserved.The attribute type information is now threaded from the CVAT task labels (which carry
input_typeon each attribute spec) through the annotation parsing pipeline via a newattr_type_mapdict. All existing callers that don't provide type info continue to work as before — the parameter defaults toNone, preserving backward compatibility.Summary by CodeRabbit