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

Update submodules #40

Merged
merged 4 commits into from
Nov 27, 2024
Merged

Conversation

ekatef
Copy link
Contributor

@ekatef ekatef commented Nov 19, 2024

As discussed during today's catchup, this PR updates the links to submodules used by the workflow to align with OET forking strategy and facilitate collaboration inside the project

@ekatef ekatef requested a review from davide-f November 19, 2024 13:18
@martacki
Copy link

Hey there, could you itemize the pros and cons of the switch?

I think this does not quite align with the "OET forking strategy" we have discussed earlier, but if there are good reasons to deviate from it, would be useful to consider and maybe reopen the discussion again.

For this particular project, there are no huge risks if we do the switch as you're also upstream developers/maintainers, but this is not true for other projects. I think we should try and setup a strategy that works everywhere to avoid creating too many different workflows...

What are your thoughts?

@ekatef
Copy link
Contributor Author

ekatef commented Nov 26, 2024

Hey @martacki thanks for looking into it.

The main reason for these changes is that maintenance of PyPSA-Earth OET fork is not aligned with the Forking Strategy. The fork main currently contains the changes implemented in a way which makes impossible contributing them into the upstream. Just to clarify, the changes of this type are needed in the upstream and we have discussed it previously, the problem is lack both of their coordination with the upstream and misalignment with the Soft Fork strategy.

Agree that the best way would be to ensure that OET fork is maintained properly and respects the OET Fork Strategy. However, it would take the significant efforts while we need to fix the existing divergence quickly as it still has lead to the confusions and wasting time in implementing improvements for geothermal. So, we envision to apply this changes as kind-of urgent fix, and work on fixing OET fork after.

@martacki
Copy link

Is this about the Pathlib dependency? I think this is currently an open PR in upstream which is maintained, but probably has some merge conflicts by now (not sure, though). I think this could be reviewed, and we can then leave all projects at their current submodule. Later we might even deviate from the whole submodule setup, but we will still keep working from forks.

  1. Deviating from the strategy and planning to come back to it later risks never actually doing this.
  2. Switching to upstream once again will set up a workflow that we already agreed on is not sustainable (we have learned that during the EEE project).
  3. Switching to upstream prevents us from learning how to deal with problems that might occur in the future, too.
  4. Switching to upsteam also risks that some of the mentioned issues will never be resolved - and they already have been lying around for a while. Maybe it's time to revisit them, resolve merge conflicts, review & merge the PR referenced above, sync the fork, which enables us to stick to the current fork strategy.

@ekatef
Copy link
Contributor Author

ekatef commented Nov 26, 2024

Thanks @martacki! I confirm that it's exactly the PR you mention which is creating troubles. Unfortunately, it does not correspond to minimal quality standards and can't be merged in its' current state.

I agree that it's not quite a sustainable strategy to keep the problems pending. But if we don't want that, then the way to go is to align the fork with the upstream taking a quick minimal-efforts route. As mentioned, the existing divergence has still lead to wasting efforts in the geothermal project.

@martacki
Copy link

As maintainer of the -Earth repo, it would be however your responsibility to leave a review that can be effectively used to improve the PR's quality standards so that it can be merged. Currently, I don't understand what we need to change in the PR to address your concern. We need more specific comments to improve.

@ekatef
Copy link
Contributor Author

ekatef commented Nov 26, 2024

As maintainer of the -Earth repo, it would be however your responsibility to leave a review that can be effectively used to improve the PR's quality standards so that it can be merged. Currently, I don't understand what we need to change in the PR to address your concern. We need more specific comments to improve.

The detailed review has been provided in this comment more than two months ago, but we didn't get any response. Moreover, there has been a detailed discussion on this PR which has been silently closed without any reaction.

@martacki
Copy link

The comment you mention is not a detailed review, it's a request split future PRs in smaller bits to ease review. Quite reasonable, and I think we have already proposed making all this happen in smaller bits in the future. But now we should deal with the existing PR.
Otherwise, qualitatively, the comment mentions that some contributions are considered useful - so I think dropping them shouldn't be the target. The target should be getting them upstream (with changes that may be proposed in a code review).

Good question though why the 2nd PR was closed. Maybe it's a duplicate of the 1st PR, or contains similar work which is now outdated?

@ekatef
Copy link
Contributor Author

ekatef commented Nov 27, 2024

Perfect! Then, I'm putting back links to both OET forks and updating the commit hash to use the latest version.

The PR to implement the experimental changes is re-opened here, and I added a link to the comments which must be addressed as the first step.

@ekatef ekatef merged commit a015227 into open-energy-transition:main Nov 27, 2024
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