DI Refactor for problem statement

[pdemro]

What does this implement/fix? Explain your changes.

There are a few reasons for this PR:

1. utils.py is becoming (has become?) a ball of mud. Moving towards a DI implementation for different modules will hopefully make the code more extensible & maintainable
2. I plan to implement a Jira adapter for the problem statement. Hence the motivation for 1. Defining/enforcing a consistent schema for a problem statement will hopefully make adding new problem statement adapters more straightforward.

Adding this as a draft PR because I'm not certain how to test all of the scenarios for the file-based problem statements. If someone can give me some sample files to test, I can take a look

[klieret]

utils.py is becoming (has become?) a ball of mud.

I don't disagree, haha.

I didn't have time to review your PR yet (we were working towards some other deadlines), but please see my comments at #760. Basically, we want to keep things incredibly simple so that SWE-agent remains easy to maintain and adapt. Anything that helps us towards that is definitely something I want to support. However, maintaining compatibility with JIRA is simply not something we can spend time on. Perhaps gitlab would be something to consider, but probably not much more than that.

But let me review this this week, talk with the team, and get back to you! I do like your refactoring in principle, especially because we will also add other sources of problem statements (that aren't issues on an online platform).

[pdemro]

@klieret This all makes sense. I'll open the PR for Jira integration separately, but I'll let you guys decide on if you want to merge. This is just the first step in that direction. I think there is an opportunity to refactor other portions of utils.py into the factory pattern as well (such as source control URL, etc)

[pdemro]

@klieret Any more thoughts on this PR? Is it worth getting conflicts resolved here for a potential merge?

[pdemro]

@klieret Any more thoughts on this PR? Is it worth getting conflicts resolved here for a potential merge?

Update: I resolved conflicts. Your team demonstrated perfectly the value of leveraging DI for adding new modules instead of caking onto the ball. A couple of todo items for organizational purpose, but please consider this PR before adding additional modules for support (like EnIGMA in this case for example)

[pdemro]

[klieret]

Hi @pdemro! Sorry for not coming back to this earlier, we were all very busy with the ICLR deadline for SWE-bench/SWE-agent M and the EnIGMA launch.

I've just found some time to think through your proposed changes. I do see what you are going for and I agree that for large projects with a lot of data sources, this might be part of the answer.

However, it also adds at least two layers of abstractions and 200 more lines. I'm also not convinced that it addresses the main sources of brittleness, such as the logic by which we determine which "issue service" to use or the fact that the task instance dictionary doesn't really have a fixed set of dictionary keys (it would have been much better to have a dataclass TaskInstance that we use to pass around the information). These things are arguably deeper in the whole framework.

However, they are also not really priorities for us at the moment.

It's honestly hard for me to say no, because you clearly spent a lot of time on this (thank you!), but as a maintainer the most important thing is to prioritize and to make sure we focus on our core (research) project. I can only recommend waiting for discussion in issues etc. before investing a lot of time...

Again, thanks a lot for the suggestion, and I know this must be frustrating, but I have to decline this.

[pdemro]

Hi @pdemro! Sorry for not coming back to this earlier, we were all very busy with the ICLR deadline for SWE-bench/SWE-agent M and the EnIGMA launch.

I've just found some time to think through your proposed changes. I do see what you are going for and I agree that for large projects with a lot of data sources, this might be part of the answer.

However, it also adds at least two layers of abstractions and 200 more lines. I'm also not convinced that it addresses the main sources of brittleness, such as the logic by which we determine which "issue service" to use or the fact that the task instance dictionary doesn't really have a fixed set of dictionary keys (it would have been much better to have a dataclass TaskInstance that we use to pass around the information). These things are arguably deeper in the whole framework.

However, they are also not really priorities for us at the moment.

It's honestly hard for me to say no, because you clearly spent a lot of time on this (thank you!), but as a maintainer the most important thing is to prioritize and to make sure we focus on our core (research) project. I can only recommend waiting for discussion in issues etc. before investing a lot of time...

Again, thanks a lot for the suggestion, and I know this must be frustrating, but I have to decline this.

Thank you very much for your thorough review @klieret . We will need to agree to disagree for the time being. I hope it can still be some help to your team. My fork will stay public for when you're ready to unpack the technical debt in these modules.