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

Updated all dependencies to the newest version. #1796

Closed
wants to merge 4 commits into from

Conversation

temotskipa
Copy link
Contributor

Please note that I changed some values in pyproject.toml to do this. Warning: do not merge if this borks OpenDevin. Run a basic test to ensure it works correctly with the new dependencies.

Copy link
Collaborator

@rbren rbren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Generally we run dependency updates through the core maintainers, for security reasons--will download and try to repro your lockfile.

I know the browsergym change will break things as-is, but we have a fix incoming

@li-boxuan
Copy link
Collaborator

@rbren I suggest we stop doing upgrade manually and only let Dependabot do it: https://docs.github.com/en/code-security/getting-started/dependabot-quickstart-guide

@temotskipa
Copy link
Contributor Author

temotskipa commented May 15, 2024

I know the browsergym change will break things as-is, but we have a fix incoming

@rbren Why? Does it conflict with other dependencies or something? Also, why tf is it stuck on a 0.1.0 release candidate anyway, when 0.2.2 is already out. Also, the PR says that you requested some changes, did you mean I need to revert the changes in pyproject.toml? Or another PR needs to be merged to introduce a fix for new browsergym versions?

@yufansong
Copy link
Collaborator

@rbren I suggest we stop doing upgrade manually and only let Dependabot do it: https://docs.github.com/en/code-security/getting-started/dependabot-quickstart-guide

I also suggest we shouldn't do it manually. Introduce dependabot is necessary.

@rbren
Copy link
Collaborator

rbren commented May 16, 2024

#1829

@temotskipa it wouldn't be super hard for an attacker to inject a vulnerable dependency into the lockfile, since that's not really getting reviewed. Dependency updates are a little scary from a security perspective 😅

That said, I do think pinning the versions here is the right thing to do, and not something dbot will solve. If you rebase your PR to fix the conflicts, I'll verify that the lockfile is safe by generating locally, and we can get this merged.

@temotskipa temotskipa reopened this May 16, 2024
@temotskipa
Copy link
Contributor Author

Please re-review and tell me clearly about any conflicts. Also would appreciate the max versions of any dependencies I can update the project to because of code-breaking changes, if any. Also looks like Dependabot is already set up, so this is probably my last PR on this topic.

@temotskipa temotskipa requested a review from rbren May 16, 2024 19:05
@neubig
Copy link
Contributor

neubig commented May 16, 2024

@rbren given that dbot is enabled, maybe this can be closed?

@temotskipa
Copy link
Contributor Author

I will be closing this since it died. I do however have another commit unrelated to this PR to pull request. I'll be opening that shortly.

@temotskipa temotskipa closed this May 18, 2024
@temotskipa temotskipa deleted the poetry branch May 18, 2024 11:46
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.

None yet

5 participants