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.

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

Fixed race condition in schema validation #2653

Merged
merged 2 commits into from
Oct 15, 2022
Merged

Fixed race condition in schema validation #2653

merged 2 commits into from
Oct 15, 2022

Conversation

tgaddair
Copy link
Collaborator

@tgaddair tgaddair commented Oct 14, 2022

Fixes an issue that occurs when running multiple threads of LudwigModel at a time:

Exception in thread Thread-2:
Traceback (most recent call last):
...
  model = LudwigModel(config)
 File "ludwig/api.py", line 224, in __init__
  self.config = merge_with_defaults(upgraded_config)
 File "ludwig/utils/defaults.py", line 310, in merge_with_defaults
  combiner_registry[config[COMBINER][TYPE]].get_schema_cls(), config[COMBINER]
 File "ludwig/schema/utils.py", line 58, in load_config_with_kwargs
  assert_is_a_marshmallow_class(cls)
 File "ludwig/schema/utils.py", line 101, in assert_is_a_marshmallow_class
  ), f"Expected marshmallow class, but `{cls}` does not have the necessary `Schema` attribute."
AssertionError: Expected marshmallow class, but `<class 'ludwig.schema.combiners.concat.ConcatCombinerConfig'>` does not have the necessary `Schema` attribute.

And:

Traceback (most recent call last):
  ...
    model = LudwigModel(config)
  File "/usr/lib/python3.7/site-packages/ludwig/api.py", line 225, in __init__
    validate_config(self.config)
  File "/usr/lib/python3.7/site-packages/ludwig/schema/__init__.py", line 83, in validate_config
    validate(instance=updated_config, schema=get_schema(), cls=get_validator())
  File "/usr/lib/python3.7/site-packages/ludwig/schema/__init__.py", line 50, in get_schema
    OUTPUT_FEATURES: get_output_feature_jsonschema(),
  File "/usr/lib/python3.7/site-packages/ludwig/schema/features/utils.py", line 84, in get_output_feature_jsonschema
    "allOf": get_output_feature_conds(),
  File "/usr/lib/python3.7/site-packages/ludwig/schema/features/utils.py", line 101, in get_output_feature_conds
    feature_schema = schema_utils.unload_jsonschema_from_marshmallow_class(schema_cls)
  File "/usr/lib/python3.7/site-packages/ludwig/schema/utils.py", line 107, in unload_jsonschema_from_marshmallow_class
    schema = js().dump(mclass.Schema())["definitions"][mclass.__name__]
  File "/usr/lib/python3.7/site-packages/marshmallow_jsonschema/base.py", line 342, in dump
    return super().dump(obj, **kwargs)
  File "/usr/lib/python3.7/site-packages/marshmallow/schema.py", line 557, in dump
    result = self._serialize(processed_obj, many=many)
  File "/usr/lib/python3.7/site-packages/marshmallow/schema.py", line 525, in _serialize
    value = field_obj.serialize(attr_name, obj, accessor=self.get_attribute)
  File "/usr/lib/python3.7/site-packages/marshmallow/fields.py", line 344, in serialize
    return self._serialize(value, attr, obj, **kwargs)
  File "/usr/lib/python3.7/site-packages/marshmallow/fields.py", line 1984, in _serialize
    return self._serialize_method(obj)
  File "/usr/lib/python3.7/site-packages/marshmallow_jsonschema/base.py", line 164, in get_properties
    schema = self._get_schema_for_field(obj, field)
  File "/usr/lib/python3.7/site-packages/marshmallow_jsonschema/base.py", line 257, in _get_schema_for_field
    schema = field._jsonschema_type_mapping()
  File "/usr/lib/python3.7/site-packages/ludwig/schema/decoders/utils.py", line 87, in _jsonschema_type_mapping
    "allOf": get_decoder_conds(feature_type),
  File "/usr/lib/python3.7/site-packages/ludwig/schema/decoders/utils.py", line 40, in get_decoder_conds
    other_props = schema_utils.unload_jsonschema_from_marshmallow_class(decoder_cls)["properties"]
  File "/usr/lib/python3.7/site-packages/ludwig/schema/utils.py", line 106, in unload_jsonschema_from_marshmallow_class
    assert_is_a_marshmallow_class(mclass)
  File "/usr/lib/python3.7/site-packages/ludwig/schema/utils.py", line 101, in assert_is_a_marshmallow_class
    ), f"Expected marshmallow class, but `{cls}` does not have the necessary `Schema` attribute."
AssertionError: Expected marshmallow class, but `<class 'ludwig.schema.decoders.sequence_decoders.SequenceTaggerDecoderConfig'>` does not have the necessary `Schema` attribute

@arnavgarg1
Copy link
Contributor

arnavgarg1 commented Oct 14, 2022

Nice! Should we also add the test you created? Would it also make sense to maybe add this to release-0.6 and release a 0.6.3?

@tgaddair
Copy link
Collaborator Author

@arnavgarg1 We could, though it may require spawning a separate python interpreter similar to the test_comet test, as once the objects have been loaded in the process memory, I suspect the issue will not appear.

@arnavgarg1
Copy link
Contributor

Got it! I guess this should be fine for now then!

@github-actions
Copy link

github-actions bot commented Oct 15, 2022

Unit Test Results

         6 files  ±  0           6 suites  ±0   3h 12m 5s ⏱️ - 24m 47s
  3 466 tests ±  0    3 344 ✔️ ±  0    77 💤 ±  0  45 ±0 
10 398 runs  +49  10 097 ✔️ +36  254 💤 +12  47 +1 

For more details on these failures, see this check.

Results for commit 01c08e2. ± Comparison against base commit b78648e.

♻️ This comment has been updated with latest results.

@tgaddair
Copy link
Collaborator Author

Looks like the determinism test is broken on master, so merging.

@tgaddair tgaddair merged commit 7c9781e into master Oct 15, 2022
@tgaddair tgaddair deleted the fix-schema-rc branch October 15, 2022 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants