Skip to content

Ensure conflict resolution is fine for ssp-dev rebase #297

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

Merged
merged 0 commits into from
Feb 19, 2025

Conversation

glatterf42
Copy link
Member

Today, @OFR-IIASA reported that we need a minimum version for sdmx1. We already have such a requirement on main, but didn't rebase ssp-dev after #295 was merged.

Upon rebasing ssp-dev on main, I ran into this merge conflict:

<<<<<<< ssp-dev
def read_sector_data(
    scenario: message_ix.Scenario, sectname: str, ssp: str | None, filename: str
=======
def get_hist_act_data(
    map_fname: str, years: list or None = None, iea_data_path: Optional[str] = None
>>>>>>> main

This likely arose because some type hints were adjusted on main such that git recognized some part of this line and thought it was just changed when in reality, the one on main was replaced entirely. I kept the version on ssp-dev, which ended up being changed again by later commits, anyway, AFAICT.

The PR also adjust a commit message that was oddly formatted.

How to review

For everyone except @khaeru: you're notified as a code owner, but this PR doesn't include any new changes that are not already on ssp-dev (except for two type hint fixes).

For @khaeru:

  • Read the diff and note that the CI checks all pass.
  • Note that we should adjust the code owners to no longer notify @measrainsey.

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
  • Add, expand, or update documentation.
  • Update doc/whatsnew.

@khaeru
Copy link
Member

khaeru commented Feb 19, 2025

Thanks for doing this. Looking at the git blame, a commit in 2024-09 by @macflo8 removed get_hist_act_data() and added read_sector_data(). So I believe keeping the former is indeed the correct choice to resolve this conflict.

@glatterf42 please force push this history to the ssp-dev branch; then we can close this PR without merging and delete this -rescue branch.

FWIW, I think the difficulty of doing these rebases and resolving these conflicts will increase, and the frequency of such conflicts will also increase, the more content sits on ssp-dev without getting merged to main. I wrote as much in #234 and #235 when creating them, but maybe haven't made enough fuss about it.

@macflo8, I know you are very busy, but perhaps these changes to .model.material.data_util, or a larger subset of the changes to the material code, are good candidates to be cherry-picked onto another branch that is easily mergeable to main. After doing so, we could rebase ssp-dev and it would shrink substantially, reducing this maintenance work.

@khaeru
Copy link
Member

khaeru commented Feb 19, 2025

  • Note that we should adjust the code owners to no longer notify @measrainsey.

Let's do this in a separate and dedicated PR.

@glatterf42 glatterf42 merged commit 5cdd868 into ssp-dev Feb 19, 2025
1 check passed
@glatterf42 glatterf42 deleted the ssp-dev-rescue branch February 19, 2025 10:28
@macflo8
Copy link
Contributor

macflo8 commented Feb 19, 2025

@macflo8, I know you are very busy, but perhaps these changes to .model.material.data_util, or a larger subset of the changes to the material code, are good candidates to be cherry-picked onto another branch that is easily mergeable to main. After doing so, we could rebase ssp-dev and it would shrink substantially, reducing this maintenance work.

Yeah, thats would indeed be good. Most of the changes might be a bit hard to disentangle from the rest, but I will take a look to identify the non-model related commits for a potential PR to main. I cannot promise to manage to do that very soon, but I will put it on my todo list.

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.

3 participants