Skip to content

improve: enhance CV validator error messages with invalid values and valid options #864

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

Open
Darner Iszat Diaz Zapana (DarnerDiaz) wants to merge 1 commit into
base: main
Choose a base branch
from
Open

improve: enhance CV validator error messages with invalid values and valid options #864

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
47 changes: 39 additions & 8 deletions cdds/cdds/common/request/validations/cv_validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,11 @@ def institution_validator(cls) -> Callable[[CVConfig, 'Request'], None]:
def validate(cv_config: CVConfig, request: 'Request'):
cv_institution = cv_config.institution(request.metadata.institution_id)
if cv_institution == 'unknown':
raise CVEntryError('Unknown "institution_id".')
valid_institutions = list(cv_config.get_collection('institution_id').keys())
raise CVEntryError(
f'Unknown institution_id "{request.metadata.institution_id}". '
f'Valid options: {valid_institutions}'
)
Comment on lines +47 to +51
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

The error message prints valid_institutions using the raw dict.keys() order and Python list repr. For readability and determinism (and to keep logs/CLI output predictable), consider sorting the options and formatting them as a comma-separated string; if the CV list can be large, consider truncating with a count (e.g., show first N + total).

Copilot uses AI. Check for mistakes.
Comment on lines +48 to +51
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

This PR’s purpose is improved error-message content, but the existing unit tests only assert that CVEntryError is raised and don’t verify the new message details (invalid value + valid options). Consider extending the validator tests to assert key substrings of the message so future refactors don’t regress these user-facing diagnostics.

Copilot uses AI. Check for mistakes.
return validate

@classmethod
Expand All @@ -59,7 +63,11 @@ def model_validator(cls) -> Callable[[CVConfig, 'Request'], None]:
def validate(cv_config: CVConfig, request: 'Request'):
source = cv_config.source(request.metadata.model_id)
if source == 'unknown':
raise CVEntryError('Unknown "model_id".')
valid_models = list(cv_config.get_collection('source_id').keys())
raise CVEntryError(
f'Unknown model_id "{request.metadata.model_id}". '
f'Valid options: {valid_models}'
)
return validate

@classmethod
Expand All @@ -74,12 +82,20 @@ def experiment_validator(cls) -> Callable[[CVConfig, 'Request'], None]:
def validate(cv_config: CVConfig, request: 'Request'):
experiment = cv_config.experiment(request.metadata.experiment_id)
if experiment == 'unknown':
raise CVEntryError('Unknown experiment_id')
valid_experiments = list(cv_config.get_collection('experiment_id').keys())
raise CVEntryError(
f'Unknown experiment_id "{request.metadata.experiment_id}". '
f'Valid options: {valid_experiments}'
)

if request.metadata.sub_experiment_id != 'none':
sub_experiment = cv_config.sub_experiment(request.metadata.sub_experiment_id)
if sub_experiment == 'unknown':
raise CVEntryError('Sub experiment not conform with CV')
valid_sub_experiments = list(cv_config.get_collection('sub_experiment_id').keys())
raise CVEntryError(
f'Unknown sub_experiment_id "{request.metadata.sub_experiment_id}". '
f'Valid options: {valid_sub_experiments}'
)
return validate

@classmethod
Expand All @@ -98,13 +114,22 @@ def validate(cv_config: CVConfig, request: 'Request'):

valid_model_types = set(model_types).issubset(set(allowed_model_types))
if not valid_model_types:
raise CVEntryError('Not all model types are allowed by the CV')
invalid_types = set(model_types) - set(allowed_model_types)
raise CVEntryError(
f'Invalid model type(s) {invalid_types}. '
f'Allowed types for experiment_id "{request.metadata.experiment_id}": {allowed_model_types}'
)
Comment on lines +117 to +121
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

invalid_types is a set, so its string representation is unordered and includes Python-specific braces (e.g., "{'AER', 'CHEM'}"). To keep the message human-friendly and stable, format these as a sorted list (or a joined string) rather than embedding the set directly.

Copilot uses AI. Check for mistakes.

required_model_types = cv_config.required_source_type(request.metadata.experiment_id)
if required_model_types:
contain_required_model_types = set(required_model_types).issubset(set(model_types))
if not contain_required_model_types:
raise CVEntryError('not all required model types are given.')
missing_types = set(required_model_types) - set(model_types)
raise CVEntryError(
f'Missing required model type(s) {missing_types} '
f'for experiment_id "{request.metadata.experiment_id}". '
f'Provided types: {model_types}'
)
Comment on lines +127 to +132
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

missing_types is a set, so the error message will be unordered and use Python set formatting. For a stable, user-friendly message, format missing types as a sorted list / comma-separated string (and consider similarly formatting model_types if you want consistent output).

Copilot uses AI. Check for mistakes.
return validate

@classmethod
Expand All @@ -123,7 +148,13 @@ def validate(cv_config: CVConfig, request: 'Request'):

cv_parent_experiment = cv_config.parent_experiment_id(experiment)
if cv_parent_experiment == 'unknown':
raise CVEntryError('Unknown parent experiment id')
raise CVEntryError(
f'Unknown parent experiment configuration for experiment_id "{experiment}". '
'Check CV configuration.'
)
if parent_experiment not in cv_parent_experiment:
raise CVEntryError('Parent experiment id does not match with id in CV config')
raise CVEntryError(
f'Invalid parent_experiment_id "{parent_experiment}" for experiment "{experiment}". '
f'Valid options: {cv_parent_experiment}'
)
return validate
Loading