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

Remove remaining global config #3099

Merged
merged 21 commits into from
Jul 26, 2024
Merged

Conversation

neubig
Copy link
Contributor

@neubig neubig commented Jul 24, 2024

What is the problem that this fixes or functionality that this introduces? Does it fix any open issues?

This removes the remaining global configuration. Finally done refactoring!

[WIP] because this still needs to be tested, please do not spend lots of time reviewing yet.

Give a summary of what the PR does, explaining any non-trivial design decisions

There are a few non-trivial things:

  1. The file storage needs to be passed around a bit
  2. The "parse arguments" function was setting the workspace_base variable, I removed this because it seems like a bit of an anti-pattern, but we probably need to make sure this doesn't break things.
  3. I decided to pass in AppConfig to Runtime. I want to avoid using AppConfig as much as possible, as it requires/provides too much information, and obscures which information is actually used by the class. However, runtimes use lots of information and different runtimes use different types of information at the moment, so this seemed necessary to avoid further extensive refactoring.

Other references

Fixes #2758

Copy link
Contributor

@xingyaoww xingyaoww left a comment

Choose a reason for hiding this comment

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

LGTM! Just a couple minor comments - i think it should be ready to merge once we fixed all the test cases

opendevin/runtime/client/runtime.py Show resolved Hide resolved
tests/unit/test_event_stream.py Outdated Show resolved Hide resolved
@neubig neubig changed the title [WIP] Remove remaining global config Remove remaining global config Jul 26, 2024
@neubig neubig marked this pull request as ready for review July 26, 2024 14:38
@neubig
Copy link
Contributor Author

neubig commented Jul 26, 2024

I have confirmed that tests pass and the app runs on my local machine, I think this is ready to merge!

@neubig neubig enabled auto-merge (squash) July 26, 2024 14:58
@neubig neubig merged commit 275ea70 into main Jul 26, 2024
@neubig neubig deleted the neubig/remove_remaining_global_config branch July 26, 2024 18:43
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.

[Refactoring]: Reduce reliance on global variables to specify configuration
3 participants