Add non_zero to common_fields.NumFCLayersField

[abidwael]

A dense encoder with 0 num_layers doesn't make sense. This PR adds the option for specifying a non_zero argument in common_fields.NumFCLayersField.

[ksbrar]

You should be able to just use ConfigValidationError

[abidwael]

It fails with the marshmallow ValidationError

[ksbrar]

That's strange. The code should be wrapping it in a ConfigValidationError. CC @justinxzhao

[ksbrar]

Actually glancing through the code we manually raise a ValidationError in quite a few locations. (Worth a separate refactoring/investigation.)

[tgaddair]

ConfigValidationError is only raised at the ModelConfig level.

[ksbrar]

Sure, but ideally this is an error that should bubble up and either be caught and wrapped by:

• JSON validation via check_schema:

  ludwig/ludwig/config_validation/validation.py

  Line 68 in f262220

  raise ConfigValidationError(f"Failed to validate JSON schema for config. Error: {error.message}")

• Deserialization via schema.load:

  ludwig/ludwig/schema/model_types/base.py

  Line 113 in f262220

  raise ConfigValidationError(f"Config validation error raised during config deserialization: {e}") from e

Am I missing something?

[justinxzhao]

Agreed it might be good to clarify some of our semantics about ConfigValidationError vs. ValidationError from marshmallow, perhaps moving all Ludwig-specific validation code to ConfigValidationError.

@abidwael It would be good to understand how the test gets through raises a marshmallow ValidationError. Perhaps it's because we don't except it in this block.

Otherwise, LGTM.

[ksbrar]

Ah @justinxzhao thanks for the link, I totally missed that <imblind.gif>

[justinxzhao]

Agreed it might be good to clarify some of our semantics about ConfigValidationError vs. ValidationError from marshmallow, perhaps moving all Ludwig-specific validation code to ConfigValidationError.

@abidwael It would be good to understand how the test gets through raises a marshmallow ValidationError. Perhaps it's because we don't except it in this block.

Otherwise, LGTM.