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

Ensure MESSAGE.enforce() runs for MESSAGE_MACRO #613

Merged
merged 4 commits into from
May 25, 2022
Merged

Ensure MESSAGE.enforce() runs for MESSAGE_MACRO #613

merged 4 commits into from
May 25, 2022

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented May 25, 2022

Closes #591 by ensuring that the MESSAGE.enforce() method, ensuring consistency of internal sets and parameters, is called when a scenario is solved using the MESSAGE_MACRO class.

Note that this demonstrates the workflow described at #603 (comment):

  1. Author opens a PR with some commits. Status in our tracker is "In progress".
  2. Identifying that their PR touches the GAMS implementation involves the nightly tests, author pushes a commit uncommenting these lines:
    # Uncomment these two lines for debugging, but leave them commented on 'main'
    # pull_request:
    # branches: [ main ]
    with a message like TEMPORARY Enable nightly workflow for PR.
  3. Author develops their PR, pushing commits, noting and ensuring that the "pytest", "lint" and "nightly" checks pass.
  4. Author requests a review; status is "In review".
  5. Reviewer does the review, possibly requesting changes.
  6. Review and author interact until reviewer is satisfied.
  7. Reviewer approves.
  8. Author or maintainer performs git rebase -i on the branch to drop the "TEMPORARY" commit from step 2.
  9. Merge.

How to review

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
  • Add, expand, or update documentation. N/A, fix bug to restore intended/documented behaviour
  • Update release notes.

@khaeru khaeru added bug Doesn't work as advertised/unintended effects macro MACRO model or MESSAGE-MACRO link labels May 25, 2022
@khaeru khaeru self-assigned this May 25, 2022
@khaeru
Copy link
Member Author

khaeru commented May 25, 2022

Prior to implementing a fix, the "nightly" CI workflow, temporarily enabled, fails: https://github.com/iiasa/message_ix/runs/6593114663#step:9:64 —as seen on the scheduled runs on main.

@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #613 (e605bfe) into main (2ae918e) will decrease coverage by 0.0%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            main    #613     +/-   ##
=======================================
- Coverage   93.0%   93.0%   -0.1%     
=======================================
  Files         43      43             
  Lines       3375    3374      -1     
=======================================
- Hits        3139    3138      -1     
  Misses       236     236             
Impacted Files Coverage Δ
message_ix/message_ix/models.py 100.0% <0.0%> (ø)

This ensures its .enforce() method is used on .run()
@khaeru
Copy link
Member Author

khaeru commented May 25, 2022

Now with the fix, the "nightly" CI workflow succeeds: https://github.com/iiasa/message_ix/runs/6593565149?check_suite_focus=true#step:9:42

(With this comment, I am making this fact visible to whoever reviews the PR.)

@khaeru
Copy link
Member Author

khaeru commented May 25, 2022

I've restarted one flaky test (failure is the in the R notebook, so unrelated), but otherwise this is complete. So now I'll request a review, with both the failing and passing runs visible.

@gidden
Copy link
Member

gidden commented May 25, 2022

LGTM - elegant solution to reduce code duplication, but does introduce the diamond problem. For now let's just note that here. I highly doubt we will ever run into it given we do not touch these classes often. A pythonic resolution to this for future searchers can be found here (if it ever becomes a problem).

Happy to merge as is - should we ignore the tutorial failure?

@gidden
Copy link
Member

gidden commented May 25, 2022

all tests pass, merging

@gidden gidden merged commit 2f48772 into main May 25, 2022
@khaeru
Copy link
Member Author

khaeru commented May 25, 2022

@gidden —thanks for the review, but the merge was a little premature.

Like the comments/description said, I was trying to demonstrate the simple workflow for running the nightly tests on a PR branch. Now we've skipped from step (7) to (9), missing (8), so changes from the "TEMPORARY" commit now appear on main.

I'll revert and see if I can fix.

khaeru added a commit that referenced this pull request May 25, 2022
khaeru added a commit that referenced this pull request May 25, 2022
Merging #613 unintentionally enabled the "nightly" workflow for all
future PR commits. Disable again.
khaeru added a commit that referenced this pull request May 25, 2022
Merging #613 unintentionally enabled the "nightly" workflow for all
future PR commits. Disable again.
@khaeru khaeru deleted the issue/591 branch May 25, 2022 19:18
@khaeru
Copy link
Member Author

khaeru commented May 25, 2022

Done via #614.

@khaeru khaeru added this to the 3.6 milestone Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Doesn't work as advertised/unintended effects macro MACRO model or MESSAGE-MACRO link
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address failing nightly workflow
2 participants