Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: DIA-1637: Check object tag compatibility for NER tags #275

Merged
merged 3 commits into from
Dec 10, 2024
Merged

fix: DIA-1637: Check object tag compatibility for NER tags #275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d7df77c
fix: DIA-1637: Check object tag compatibility for NER tags
Dec 9, 2024
3be60d3
fix logic to handle when allowed_control_tags is None
Dec 9, 2024
c40fb95
black
matt-bernstein Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions adala/skills/collection/label_studio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class LabelStudioSkill(TransformSkill):
label_config: str = "<View></View>"
allowed_control_tags: Optional[list[str]] = None
allowed_object_tags: Optional[list[str]] = None

# TODO: implement postprocessing to verify Taxonomy

@cached_property
Expand All @@ -53,7 +53,14 @@ def ner_tags(self) -> List[ControlTag]:
for tag_name in control_tag_names:
tag = self.label_interface.get_control(tag_name)
if tag.tag.lower() in {"labels", "hypertextlabels"}:
tags.append(tag)
if self.allowed_object_tags:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to remember why we don't just always pass allowed_object_tags... do we have a good reason that this shouldn't always just be set, or have we kinda just stumbled into having to support both of these cases?
(I know we're not gonna fix it as part of this PR if that's the case, just want to make sure I understand here)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/HumanSignal/label-studio-enterprise/blob/develop/label_studio_enterprise/lse_ml_models/skills.py#L475

This is the reason I guess. I dont know the actual details of how passing it would drop information, since we only save filtered field_schema and keep the entire label_config anyway

if all(
object_tag.tag in self.allowed_object_tags
for object_tag in tag.objects
):
tags.append(tag)
else:
tags.append(tag)
return tags

@cached_property
Expand All @@ -68,14 +75,13 @@ def image_tags(self) -> List[ObjectTag]:
if tag.tag.lower() == "image":
tags.append(tag)
return tags



def __getstate__(self):
"""Exclude cached properties when pickling - otherwise the 'Agent' can not be serialized in celery"""
state = deepcopy(super().__getstate__())
# Remove cached_property values
for key in ['label_interface', 'ner_tags', 'image_tags']:
state['__dict__'].pop(key, None)
for key in ["label_interface", "ner_tags", "image_tags"]:
state["__dict__"].pop(key, None)
return state

@model_validator(mode="after")
Expand Down
2 changes: 1 addition & 1 deletion server/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ async def submit_streaming(request: SubmitStreamingRequest):
"""

task = streaming_parent_task

result = task.apply_async(
kwargs={"agent": request.agent, "result_handler": request.result_handler}
)
Expand Down
12 changes: 6 additions & 6 deletions tests/test_serialization.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,11 @@ def test_agent_is_pickleable():
"timeout_ms": 1000,
},
"skills": [
{
"name": "label_studio_skill",
"type": "LabelStudioSkill",
"input_template": "Classify sentiment of the input text: {input}",
"label_config": """
{
"name": "label_studio_skill",
"type": "LabelStudioSkill",
"input_template": "Classify sentiment of the input text: {input}",
"label_config": """
<View>
<Text name="text" value="$text" />
<Choices name="output" toName="text">
Expand All @@ -161,7 +161,7 @@ def test_agent_is_pickleable():
<Choice value="neutral" />
</Choices>
</View>
"""
""",
}
],
}
Expand Down
10 changes: 7 additions & 3 deletions tests/test_stream_inference.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
<Choice value="neutral" />
</Choices>
</View>
"""
""",
}
],
}
Expand Down Expand Up @@ -168,11 +168,15 @@ async def test_run_streaming(
)

# Verify that producer is called with the correct amount of send_and_wait calls and data
assert mock_kafka_producer.send_and_wait.call_count == 1, f"Expected 1 call but got {mock_kafka_producer.send_and_wait.call_count}"
assert (
mock_kafka_producer.send_and_wait.call_count == 1
), f"Expected 1 call but got {mock_kafka_producer.send_and_wait.call_count}"
try:
mock_kafka_producer.send_and_wait.assert_any_call(
"output_topic", value=TEST_OUTPUT_DATA
)
except AssertionError as e:
actual_calls = mock_kafka_producer.send_and_wait.call_args_list
raise AssertionError(f"Expected call with ('output_topic', value={TEST_OUTPUT_DATA}) but got:\n{actual_calls}") from e
raise AssertionError(
f"Expected call with ('output_topic', value={TEST_OUTPUT_DATA}) but got:\n{actual_calls}"
) from e
Loading