-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
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? |
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. |
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.
|
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. |
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. |
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. 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? |
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. |
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