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

feat: Improve conversion error handling & add migration guide. #304

Merged
merged 3 commits into from
Dec 16, 2024

Conversation

jetuk
Copy link
Member

@jetuk jetuk commented Dec 3, 2024

Refactor the conversion errors and make them available as Python objects. This allows handling the conversion error cases much more user friendly.

Add an initial migration guide and example listing the demonstrates how to use the conversion function and handle the resulting errors.

Some other minor error updates and improvements included.

@jetuk jetuk requested a review from Batch21 December 3, 2024 16:59
Refactor the conversion errors and make them available as Python
objects. This allows handling the conversion error cases much
more user friendly.

Add an initial migration guide and example listing the demonstrates
how to use the conversion function and handle the resulting errors.

Some other minor error updates and improvements included.
Copy link
Contributor

@Batch21 Batch21 left a comment

Choose a reason for hiding this comment

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

Applied this to converting a reasonably complex model and it was really helpful.

A few minor suggestions on the migration guide but otherwise looks good to go!

The conversion function that takes a JSON string and returns a tuple of the converted JSON string and a list of errors.
The function then handles these errors using the `handle_conversion_error` function.
After the errors are handled other arbitrary changes are applied using the `patch_model` function.
Finally, the converted JSON can be saved to a new file and run using Pywr v2.x.
Copy link
Contributor

Choose a reason for hiding this comment

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

The block above might be clearer as a numbered list?


Pywr v2.x includes a more strict schema for defining models. This schema, along with the
[pywr-v1-schema](https://crates.io/crates/pywr-v1-schema) crate, provide a way to convert models from v1.x to v2.x.
However, this process is not perfect and will more than likely require manual intervention to complete the migration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe emphasize that its an iterative process and may take some time to complete for complex models

ignoring them.
It is suggested to implement a function that can handle these errors in a way that is appropriate for your use case.
Begin by matching a few types of errors and then expand the matching as needed. By raising exceptions
for unhandled errors, you can ensure that all errors are eventually accounted, and that new errors are not missed.
Copy link
Contributor

Choose a reason for hiding this comment

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

should be '...accounted for, ' ?

@jetuk jetuk requested a review from Batch21 December 16, 2024 11:01
@jetuk jetuk merged commit 17c7784 into main Dec 16, 2024
7 checks passed
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.

2 participants