Skip to content

feat: raise UnsupportedExtensionException when loading .yml files#2890

Open
ndormann wants to merge 1 commit into
facebookresearch:mainfrom
ndormann:feat/exception_on_invalid_file
Open

feat: raise UnsupportedExtensionException when loading .yml files#2890
ndormann wants to merge 1 commit into
facebookresearch:mainfrom
ndormann:feat/exception_on_invalid_file

Conversation

@ndormann

@ndormann ndormann commented Apr 11, 2024

Copy link
Copy Markdown

Motivation

When trying to open a config with the unsupported extension .yml a misleading MissingConfigException is returned.
For details see #2889
The proposed change raises a UnsupportedExtensionException with a helpful error message.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

The test test_load_yml_file has been adapted to check that the exception is raised correctly.

Related Issues and PRs

#2889

@ndormann ndormann force-pushed the feat/exception_on_invalid_file branch from 1af7b56 to 4e87b94 Compare April 11, 2024 01:05
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 11, 2024
@ndormann ndormann force-pushed the feat/exception_on_invalid_file branch from 4e87b94 to 7f3bc1d Compare April 11, 2024 14:01
@ndormann ndormann changed the title feat: raise UnsupportedExtensionError when loading .yml files feat: raise UnsupportedExtensionException when loading .yml files Apr 11, 2024

@Jasha10 Jasha10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you @ndormann.

config_search_path=create_config_search_path(path)
)
with raises(UnsupportedExtensionException):
cfg = config_loader.load_configuration(

Check warning

Code scanning / CodeQL

Variable defined multiple times

This assignment to 'cfg' is unnecessary as it is [redefined](1) before this value is used.
Comment thread hydra/errors.py
class HydraDeprecationError(HydraException): ...


class UnsupportedExtensionException(HydraException): ...

Check notice

Code scanning / CodeQL

Statement has no effect

This statement has no effect.
@Jasha10 Jasha10 linked an issue Apr 11, 2024 that may be closed by this pull request
2 tasks
@ndormann

Copy link
Copy Markdown
Author

@Jasha10 what is the status on merging this?

@omry omry added the awaiting_response Awaiting response label Jun 8, 2026
@omry omry added this to the Hydra 1.4.0 milestone Jun 8, 2026

@omry omry left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you for the PR. This is a real bugfix and I would like to get it into 1.4, but I need a few changes before merging.

Please update the PR to:

  • Add a news fragment, likely news/2889.bugfix.
  • Assert the exception message in the test, not only the exception type.
  • Either use an existing Hydra exception with the clearer message, or explain why this needs a new public exception type.

Once those are addressed, I can take another look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting_response Awaiting response CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Misleading error message when opening .yml file

5 participants