Validate constraint parameters in add_constraints

[fealho]

Resolve #1210.

[codecov-commenter]

Codecov Report

Base: 95.66% // Head: 95.68% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (cefc1c6) compared to base (5f586ec).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head cefc1c6 differs from pull request most recent head e629ea0. Consider uploading reports for the commit e629ea0 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@              Coverage Diff               @@
##           V1.0.0.dev    #1233      +/-   ##
==============================================
+ Coverage       95.66%   95.68%   +0.02%     
==============================================
  Files              47       47              
  Lines            3618     3637      +19     
==============================================
+ Hits             3461     3480      +19     
  Misses            157      157              

Impacted Files	Coverage Δ
sdv/data_processing/data_processor.py	96.91% <100.00%> (+0.07%)	⬆️
sdv/multi_table/base.py	99.44% <100.00%> (+0.03%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pvk-developer]

It feels like we are doing this twice, I think we can just check if table_name is not in constraint_dict raise the error stating that you are missing table_name, if not then the data processor will raise the error.

[fealho]

Yeah, I did it like that so the errors would show up together (if you don't pass any parameters, it will crash saying only table_name is missing, and only after you add table_name will it tell you the other errors).

But I think your way is better, it's too much duplicated code otherwise.

[fealho]

@amontanez24 @npatki Note that these errors are not being aggregated with the other constraint errors (AggregateConstraintsError, InvalidConstraintsError). So these will show up first, and only after it is fixed the other aggregate errors will show up.

This seems to be what the issue was describing, but I wanted to double check this is indeed the intended approach.

[npatki]

@fealho

Note that these errors are not being aggregated with the other constraint errors

The aggregated constraints errors are thrown when are doing fit right? Ie they happen in the middle of execution when trying to model the constraint.

This issue is just when specifying the constraint before any sort of processing or fitting happens.

[fealho]

@npatki They are thrown in the add_constraints as an InvalidConstraintsError:

SDV/sdv/data_processing/data_processor.py

Lines 233 to 254 in 5f586ec

	def add_constraints(self, constraints):
	"""Add constraints to the data processor.
	Args:
	constraints (list):
	List of constraints described as dictionaries in the following format:
	* ``constraint_class``: Name of the constraint to apply.
	* ``constraint_parameters``: A dictionary with the constraint parameters.
	"""
	errors = []
	validated_constraints = []
	for constraint_dict in constraints:
	constraint_dict = deepcopy(constraint_dict)
	try:
	self._validate_constraint_dict(constraint_dict)
	validated_constraints.append(constraint_dict)
	except (AggregateConstraintsError, InvalidConstraintsError) as e:
	reformated_errors = '\n'.join(map(str, e.errors))
	errors.append(reformated_errors)
	if errors:
	raise InvalidConstraintsError(errors)

[npatki]

@fealho oh got it!

In any case, I think the answer remains the same: We want to throw the errors described in this issue first. And then if that all passes, we can then do additional validation and throw an AggregateConstraintsError as needed.

[amontanez24]