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

Return REopt errors warns back to user #370

Merged
merged 31 commits into from
Jan 3, 2023
Merged

Return REopt errors warns back to user #370

merged 31 commits into from
Jan 3, 2023

Conversation

rathod-b
Copy link
Collaborator

@rathod-b rathod-b commented Sep 26, 2022

Please check if the PR fulfills these requirements

  • CHANGELOG.md is updated
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

(Bug fix, feature, docs update, ...)
Bug fix and feature

What is the current behavior?

(You can also link to an open issue here)
#371
Currently, some fields default to the previous iteration of defaults. Those have been identified and corrected.

#363
Currently, there is no ability for REopt.jl error messages to be passed back to the user in case of an error. This PR adds this functionality to REopt API's JOB app.

What is the new behavior (if this is a feature change)?

When there is an error or warning from REopt.jl, it is included under Messages key along with optimization response. Multiple errors and warnings can be accommodated in the API response.

Documentation available here: https://github.com/NREL/REopt-API-Analysis/wiki/Common-Errors

Does this PR introduce a breaking change?

(What changes might users need to make in their application due to this PR?)
No. Users now receive additional information in their output JSON which they can use as appropriate.

Other information:

New test

  • Included a test to exercise all possible REopt API inputs.

@rathod-b rathod-b self-assigned this Sep 26, 2022
@rathod-b rathod-b linked an issue Sep 26, 2022 that may be closed by this pull request
3 tasks
@@ -0,0 +1,270 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

You know the default description that gets created for PRs in this repo that you fill in? In the "Please check if the PR fulfills these requirements" section, could you add a checkbox that says something like "Any new Django model inputs have also been added to job/test/posts/all_inputs_test.json"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the PR description.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant editing the default description that gets created for every PR. Adding to the 3 checkboxes at the top. That way for future PRs, people check that they have added to all_inputs_test.json.

Copy link
Collaborator

Choose a reason for hiding this comment

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

added issue #394 for this so I could approve this PR

job/models.py Show resolved Hide resolved
job/models.py Outdated Show resolved Hide resolved
job/views.py Outdated Show resolved Hide resolved
job/src/process_results.py Outdated Show resolved Hide resolved
job/src/run_jump_model.py Show resolved Hide resolved
julia_src/Manifest.toml Outdated Show resolved Hide resolved
julia_src/http.jl Show resolved Hide resolved
julia_src/http.jl Show resolved Hide resolved
julia_src/http.jl Show resolved Hide resolved
@hdunham
Copy link
Collaborator

hdunham commented Oct 18, 2022

Could you also fill out the PR description and update the changelog?

@Bill-Becker
Copy link
Collaborator

@rathod-b this would be great to merge soon. What's your timeline for responding to @hdunham's review comments?

Address PR#370 comments, update changelog, consolidate migration files, rename MessageOutputs to REoptjlMessageOutputs to differentiate. Update how errors and warns are returned to the user using existing Messages key, update error and informational text around error handling
Create a dictionary of errors and warnings to they are easy to process by API users. always return an empty dictionary if there are no errors/warnings. Update tests
@rathod-b
Copy link
Collaborator Author

@Bill-Becker @hdunham This PR should be ready for review next week (I just need to add documentation to REopt Wiki). Note that we would need to merge NREL/REopt.jl#118 before this can be merged into Develop.

@rathod-b rathod-b marked this pull request as ready for review November 29, 2022 19:20
@rathod-b
Copy link
Collaborator Author

Some CHP tests are failing most likely because REopt TOML files point to REopt.jl's error-msgs branch. When that branch is merged into develop, failing tests should pass.

@Bill-Becker
Copy link
Collaborator

@rathod-b looks like the chp_defaults test is not passing because the stuff starting here:

if haskey(d, "CHP")
did not get merged when you merged develop.

@Bill-Becker
Copy link
Collaborator

@rathod-b I fixed a couple of accidental auto-merge deletion issues, and all the tests are passing now. I would comb through everything to make sure there are not more unintended deletions or other merge issues which may not be causing any issues with tests. Otherwise, looks good. Great work on this!

@Bill-Becker Bill-Becker requested a review from hdunham December 8, 2022 18:47
@Bill-Becker
Copy link
Collaborator

@rathod-b The error-messages branch PR has been merged into master and published. Can you update the REopt.jl package in this branch?

@rathod-b
Copy link
Collaborator Author

@rathod-b The error-messages branch PR has been merged into master and published. Can you update the REopt.jl package in this branch?

Yes, i can. Will let you know when this is updated, re-tested, and validated.

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.

Pass REopt.jl errors/warns to API v3 requestor
4 participants