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

Add dependabot dependent version updates #1064

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

paulf81
Copy link
Collaborator

@paulf81 paulf81 commented Feb 11, 2025

Add dependabot dependent version updates

This PR introduces automated updating of dependent versions via github's dependabot

Following basic settings from here:

https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/configuring-dependabot-version-updates

When this file is in github then dependabot should generate PRs we can consider to update dependencies. This should save us from having to do this manually.

Could consider patching this into main as well.

@paulf81 paulf81 requested review from misi9170 and rafmudaf February 11, 2025 21:04
@paulf81 paulf81 self-assigned this Feb 11, 2025
@rafmudaf
Copy link
Collaborator

Oooh that's a spicy one I think. My knee-jerk reaction is a strong "heck no". However, I'm happy to have the discussion and I can definitely be convinced. @paulf81 what's the context and motivation for this?

@paulf81
Copy link
Collaborator Author

paulf81 commented Feb 11, 2025

Oooh that's a spicy one I think. My knee-jerk reaction is a strong "heck no". However, I'm happy to have the discussion and I can definitely be convinced. @paulf81 what's the context and motivation for this?

Thanks for be open to discussion! So here's my pitch points:

  1. Saves us from having to ourselves (or our users) identify when our dependencies are obsolete: (eg Update coloredlogs requirement #939 or more recently Update numpy req to 2 #1051)
  2. Automatically generates the PR with the fix which we can review, saving some time for us to write these ourselves
  3. Doesn't actually change any code if we don't accept the PR. We can just note in the PR we don't want to make the change now and close the request

@misi9170
Copy link
Collaborator

misi9170 commented Feb 12, 2025

@rafmudaf that was my original reaction too! But I think @paulf81 has a point---it will only open a PR (which we can always reject if needed). If we find that this is happening too often or is overbearing, we can always remove it again.

@paulf81 , do you know where the branches that these PRs are for will live? Will they be on NREL/floris? Or on some other fork somehow?

@misi9170
Copy link
Collaborator

By my reading, it looks like PRs will be opened targeting the main branch, rather than the develop branch. Perhaps this is correct---if there is a true dependency conflict, it may need to be resolved in a patch release. Otherwise, it looks like we can add target-branch: "develop" to the dependabot.yml to have it open PRs to the develop branch.

@paulf81
Copy link
Collaborator Author

paulf81 commented Feb 12, 2025

By my reading, it looks like PRs will be opened targeting the main branch, rather than the develop branch. Perhaps this is correct---if there is a true dependency conflict, it may need to be resolved in a patch release. Otherwise, it looks like we can add target-branch: "develop" to the dependabot.yml to have it open PRs to the develop branch.

Good catch, added

@paulf81
Copy link
Collaborator Author

paulf81 commented Feb 12, 2025

If we find that this is happening too often or is overbearing, we can always remove it again.

Or move it from weekly to monthly

@rafmudaf
Copy link
Collaborator

Updating dependency versions can have unintended consequences. When things go well, the consequences are positive - improved performance, less memory consumption, improved integration with the rest of the ecosystem. However, when things go bad, it can be difficult to diagnose and the problems may not show up immediately. This is especially true for lower level libraries like Numpy, attrs, or numexpressions - the ones that do things that look like magic and seem to "just work". For this reason, my approach to software stability is to fix something when it's needed but never before that.

@paulf81 @misi9170 What are your thoughts on the process for evaluating whether an updated library version is worth incorporating? I anticipate that the reviews for many of these automated pull requests will be along the lines of "Tests pass, looks good to me", but this approach does not consider the qualitative changes the come with a new library version.

Some things that might help us get to middle ground are:

  • Establishing a process for evaluating a dependency version update and the associated pull request
  • Reducing the frequency of checks to "monthly"; "quarterly" would be better but it's not an option
  • Configure the update-types option to skip major versions
  • Configure the versioning-strategy option

The full options list is at the options reference page.

@paulf81
Copy link
Collaborator Author

paulf81 commented Feb 12, 2025

hi @rafmudaf, these are good points, thanks for them! I guess my main thinking is these concerns mainly apply to the point we decided to merge a PR. Implementing this change will only create the PR, and we could have a policy of "good to know" and simply close 90% of them according to a conservative approach of only updating failing packages.

I think there's a good motivation to be made aware at least of when packages we import have moved a version. It's usually a heads up that conflicts will be coming sooner or later as eventually more and more of the ecosystem makes the jump. Take for example the numpy -> 2 PR. We could fairly claim that there was "no problem" in the sense the FLORIS itself works fine with numpy 1, but as one user commented our back dating was making complicated problems for them:

I think there will be more difficulties for the majority of users if you do not upgrade to numpy 2.x than if you upgrade to numpy 2.x. There are less and less packages not compatible with numpy 2.0, Floris is one of them...
I think it's better to encourage users to upgrade to numpy 2 (for their own good) if they want to use new floris functionalities than to keep them in their "dead end" with numpy 1.20...

So my main argument is that since we aren't forced to merge the PRs, at least gaining the information they provide by their creation is useful and keeps us from having to learn these manually or through user-submitted issues.

Then going through your other points:

  • Establishing a process for evaluating a dependency version update and the associated pull request

This will probably be case by case. Numpy/pandas being probably merit a big discussion/think, where coloredlogs can be simpler.

  • Reducing the frequency of checks to "monthly"; "quarterly" would be better but it's not an option

Dang

This won't work, FLORIS is configured to automatically accept all minor and patch versions within a major version. The only thing that dependabot can alert us to our major versions.This also implies there won't be very many of these regardless of the weekly setting

No strong feeling on this if you'd like to propose something?

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