Skip to content

Add workspace setting in UI #1448

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

Closed
wants to merge 4 commits into from

Conversation

johnnyaug
Copy link
Contributor

@johnnyaug johnnyaug commented Apr 29, 2024

Resolves #1145.

This feature allows OpenDevin to operate in the immediate subdirectories of the WORKSPACE_BASE directory.
The immediate children of WORKSPACE_SUBDIR now appear in the settings, and the OpenDevin agent will consider the selected directory as the working directory on the following tasks.

@@ -61,7 +62,7 @@ def __init__(
raise ex

self.instance_id = sid if sid is not None else str(uuid.uuid4())

self.workspace_mount_path = workspace_mount_path or config.get(ConfigType.WORKSPACE_MOUNT_PATH)
Copy link
Collaborator

@rbren rbren Apr 29, 2024

Choose a reason for hiding this comment

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

So this isn't quite the logic I think we should have.

Instead, WORKSPACE_MOUNT_PATH should be held constant, and the sandboxes should accept a relative directory, like ./app`

This way the user has a guarantee that we'll always mount on WORKSPACE_MOUNT_PATH or a subdirectory

@rbren
Copy link
Collaborator

rbren commented Apr 29, 2024

Thanks for taking this on! Excited to add this bit

@johnnyaug johnnyaug force-pushed the add_workspace_setting branch from b9aa55a to 8cd7e47 Compare May 3, 2024 15:44
@codecov-commenter
Copy link

codecov-commenter commented May 5, 2024

Codecov Report

Attention: Patch coverage is 63.15789% with 14 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@755a407). Click here to learn what that means.

Files Patch % Lines
opendevin/server/listen.py 0.00% 8 Missing ⚠️
opendevin/runtime/files.py 0.00% 5 Missing ⚠️
opendevin/server/agent/agent.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1448   +/-   ##
=======================================
  Coverage        ?   64.24%           
=======================================
  Files           ?       99           
  Lines           ?     4072           
  Branches        ?        0           
=======================================
  Hits            ?     2616           
  Misses          ?     1456           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@neubig
Copy link
Contributor

neubig commented May 11, 2024

Hey @johnnyaug , thanks for this! It seems that this is now out of sync, so if you could bring it back in sync with main we can take a look.

@johnnyaug
Copy link
Contributor Author

Hey @neubig, sure - on it

@johnnyaug johnnyaug force-pushed the add_workspace_setting branch 2 times, most recently from 6250690 to 42da978 Compare May 12, 2024 12:31
*/
export const getSettingsDifference = (settings: Partial<Settings>) => {
const currentSettings = getSettings();
export const getSettingsDifference = (
Copy link
Contributor Author

@johnnyaug johnnyaug May 12, 2024

Choose a reason for hiding this comment

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

This change is for testability: The test for SettingsModal mocks getSettings.
Unfortunately, the internal call to getSettings from getSettingsDifference could not be mocked, because of this.

I think the new way is also better in terms of separation of concerns.

@johnnyaug johnnyaug marked this pull request as ready for review May 12, 2024 13:22
@johnnyaug johnnyaug requested a review from rbren May 12, 2024 13:22
@johnnyaug johnnyaug force-pushed the add_workspace_setting branch 2 times, most recently from 0c01662 to 494c1fe Compare May 13, 2024 11:08
Comment on lines 39 to 51
return DockerExecBox(
sid=sid, timeout=config.sandbox_timeout, workspace_mount_path=workspace
)
elif sandbox_type == 'local':
return LocalBox(timeout=config.sandbox_timeout)
return LocalBox(timeout=config.sandbox_timeout, workspace=workspace)
elif sandbox_type == 'ssh':
return DockerSSHBox(sid=sid, timeout=config.sandbox_timeout)
return DockerSSHBox(
sid=sid, timeout=config.sandbox_timeout, workspace_mount_path=workspace
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should call the input parameter here the same thing, since these are all Sandbox objects. Maybe relative_workspace?

Then the mount path would be path.join(config.workspace_mount_path, relative_workspace)

I think it's also a good idea to keep the extra workspace directory param you're adding relative, rather than an absolute path. The base path varies between in-docker and out-of-docker, so it'd be nice to have a consistent reference to a subdirectory that's valid in both places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - done!

@johnnyaug johnnyaug force-pushed the add_workspace_setting branch from a63cc13 to 350bdb1 Compare May 14, 2024 09:16
@rbren
Copy link
Collaborator

rbren commented May 14, 2024

Sorry @johnnyaug I should have tried to get this in before my PR.

The plumbing is a little different now--the runtime is instantiated outside the AgentController. Hopefully not too much work to merge

@johnnyaug johnnyaug force-pushed the add_workspace_setting branch from 350bdb1 to 9668472 Compare May 15, 2024 15:07
@johnnyaug johnnyaug requested a review from rbren May 15, 2024 15:14
@@ -71,6 +81,9 @@ def __init__(
self.event_stream.subscribe(EventStreamSubscriber.RUNTIME, self.on_event)
self._bg_task = asyncio.create_task(self._start_background_observation_loop())

def set_workspace_subdir(self, workspace_subdir: str):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll admit it's not the best practice to change the state on an otherwise fixed-state object, so I'm happy to hear your suggestions.

@johnnyaug
Copy link
Contributor Author

johnnyaug commented May 16, 2024

@rbren, no worries - I hope this cuts it, please take another look

@rbren
Copy link
Collaborator

rbren commented May 16, 2024

@johnnyaug unfortunately the agent appears to continue editing the root directory even when I select a subdirectory...

Every time we save new settings, the agent controller and runtime are recreated--maybe instead of setting subdir on the fly, it should be passed directly into the sandbox init?

@johnnyaug johnnyaug force-pushed the add_workspace_setting branch from 9e12aa4 to f879e19 Compare May 16, 2024 13:44
@johnnyaug johnnyaug force-pushed the add_workspace_setting branch 4 times, most recently from ff9d91b to 3ebd638 Compare May 28, 2024 09:29
johnnyaug added 2 commits June 2, 2024 11:50
allow runcmd under workspace subdir

wip

wip

wip

Revert "wip"

This reverts commit 135770ca5e0170a36827d4a1671b5e13fc944633.

wip

fix test

test caught this!

some reverts

fix upload

linta
@johnnyaug johnnyaug force-pushed the add_workspace_setting branch from a950ef1 to 9802ff1 Compare June 2, 2024 08:52
@johnnyaug
Copy link
Contributor Author

@rbren, could you please take another look?

@mamoodi
Copy link
Collaborator

mamoodi commented Jun 4, 2024

Hi @johnnyaug, I have been tasked with testing this PR and will try to keep on top of testing it when you make changes. I'm running on MacOS. Here is my current structure:

  • workspace root
    • test-dir
      • booboo.rb
      • deep dir
    • another test-dir
      • file.rb

There's a couple of issues:

  • A lot of the times when I select another folder, it gives me this error
    Screenshot 2024-06-04 at 11 24 30 AM
  • If I switch to test-dir folder and then to deep dir folder, it creates a deep dir folder at the workspace root. This seems to be an issue.
  • If I want to select deep dir, I first have to select test-dir then let the Agent initialize. Then select deep dir because this is two levels deep. In general what happens when I want to select a directory 6 levels deep. CC: @rbren

I tested the Workspace root and one level deep directories and it works well.

What I suggest is in the PR description, you describe how you expect the behavior of setting a new workspace directory should work. For example how you expect setting a directory one level deep vs three level deep should work. Make sure maintainers approve of that and then work out the issues. I will continue testing it as you resolve the issues as this seems like it would be very valuable.

@rbren
Copy link
Collaborator

rbren commented Jun 4, 2024

IMO we only really need to go one directory deep here. I think the best experience would be:

  • Start in the root dir
  • In settings, provide options for:
    • /
    • /foo
    • /bar
    • etc

@mamoodi
Copy link
Collaborator

mamoodi commented Jun 4, 2024

IMO we only really need to go one directory deep here. I think the best experience would be:

  • Start in the root dir

  • In settings, provide options for:

    • /
    • /foo
    • /bar
    • etc

@johnnyaug in that case things will be simpler. So if you have this directory structure:

  • Workspace Root
    • sub-folder-1
      • deep-folder-1
    • sub-folder-2
      • deep-folder-2
    • sub-folder-3

The UI will always remain the same and always shows:

  • Workspace Root
  • sub-folder-1
  • sub-folder-2
  • sub-folder-3

Even if you select sub-folder-1.

I believe you have most of it working. Just need to address the error and always only show the level1 folders.

@johnnyaug
Copy link
Contributor Author

@rbren @mamoodi Just so we're all on the same page - the feature allows exactly what @rbren is suggesting and allows to go only one level deep in the setting. When using the file explorer, you can go as many levels deep as you like, but this is unrelated to this feature.

@mamoodi Hey and thank you for looking into it!
I added the PR description, I hope this makes sense.

In your comments, when you refer to "selecting another directory" I wasn't sure whether you meant using the new subdirectory setting, or selecting using the file explorer. Please try to be unambiguous about it in the future.

Anyway I will take a look at the issues you're describing and report back here.

@mamoodi
Copy link
Collaborator

mamoodi commented Jun 5, 2024

@johnnyaug my apologies if I wasn't clear! I am only using the new dropdown added in settings called "OpenDevin Workspace directory". I am unsure what you mean by using the file explorer.

My Workspace structure is like so:

  • workspace root
    • test-dir
      • booboo.rb
      • deep dir

Notice that I've already selected test-dir for my workspace but now it allows me to select deep dir which is two levels deep compared to my workspace root.
Screenshot 2024-06-05 at 8 06 20 AM

@tobitege
Copy link
Collaborator

tobitege commented Jun 5, 2024

edit: comment outdated, removed.

@neubig
Copy link
Contributor

neubig commented Jun 26, 2024

Hi! @johnnyaug , are you still interested in this? If so we'd appreciate getting it in, but if not maybe we can close the PR.

@mamoodi
Copy link
Collaborator

mamoodi commented Jun 30, 2024

I'm going to close this PR for now. If you happen to continue the work, please reopen the PR again. Thank you.

@mamoodi mamoodi closed this Jun 30, 2024
@SmartManoj
Copy link
Contributor

SmartManoj commented Jul 11, 2024

image

Before changing:

image

After changing:

image
image

@mamoodi, I couldn't reproduce it.
I selected test-dir and click saved. Then I reopen the settings, I got a tick sign in the dropdown values for the selected one and deep dir is not listed for me. Did I miss any step?

@mamoodi
Copy link
Collaborator

mamoodi commented Jul 11, 2024

@SmartManoj refresh the browser containing OpenDevin and see if it shows up.

try:
return request.state.session.agent_session.runtime.file_store.list(path)
files: list[str] = request.state.session.agent_session.runtime.file_store.list(
Copy link
Contributor

@SmartManoj SmartManoj Jul 11, 2024

Choose a reason for hiding this comment

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

After selecting a sub_dir, file_store points to the sub_directory.
Then after a refresh, it lists the sub_dir's next-level folders. Should there be a separate option (as the session will not be started if any existing session) end-point and a file_store to fetch the workspace_base folders?

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.

Setting for workspace directory in the UI
7 participants