OpenHands icon indicating copy to clipboard operation
OpenHands copied to clipboard

remove screenshot in browser observation

Open frankxu2004 opened this issue 1 year ago • 1 comments

Put all the removal of screenshot logic inside to_memory for browser obs.

Fixes #1553

frankxu2004 avatar May 05 '24 08:05 frankxu2004

Codecov Report

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

:exclamation: No coverage uploaded for pull request base (main@16400e4). Click here to learn what that means.

Files Patch % Lines
opendevin/events/observation/browse.py 66.66% 3 Missing :warning:
opendevin/events/utils.py 75.00% 3 Missing :warning:
agenthub/delegator_agent/agent.py 0.00% 2 Missing :warning:
opendevin/controller/agent_controller.py 83.33% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1588   +/-   ##
=======================================
  Coverage        ?   62.59%           
=======================================
  Files           ?       92           
  Lines           ?     3735           
  Branches        ?        0           
=======================================
  Hits            ?     2338           
  Misses          ?     1397           
  Partials        ?        0           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar May 05 '24 08:05 codecov-commenter

Just updated!

On Sun, May 5, 2024 at 08:18 Robert Brennan @.***> wrote:

@.**** approved this pull request.

In opendevin/events/observation/browse.py https://github.com/OpenDevin/OpenDevin/pull/1588#discussion_r1590297727:

@@ -5,35 +5,65 @@ from .observation import Observation

+def _remove_fields(obj, fields: set[str]):

might make sense to put _remove_fields in the events folder so other actions/observations can use it

In opendevin/events/observation/browse.py https://github.com/OpenDevin/OpenDevin/pull/1588#discussion_r1590297789:

  • elif isinstance(obj, list) or isinstance(obj, tuple):
  •    for item in obj:
    
  •        _remove_fields(item, fields)
    
  • elif hasattr(obj, 'dataclass_fields'):
  •    for field in fields:
    
  •        if field in obj.__dataclass_fields__:
    
  •            setattr(obj, field, None)
    
  •    for value in obj.__dict__.values():
    
  •        _remove_fields(value, fields)
    

do we need this part? Seems like the first if covers everything

— Reply to this email directly, view it on GitHub https://github.com/OpenDevin/OpenDevin/pull/1588#pullrequestreview-2039675305, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABTNCYWLK43SUTGSJO7EOD3ZAYPSDAVCNFSM6AAAAABHHQMXZOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMZZGY3TKMZQGU . You are receiving this because you authored the thread.Message ID: @.***>

frankxu2004 avatar May 05 '24 15:05 frankxu2004

Looks like a couple integration tests are unhappy for some reason...

rbren avatar May 06 '24 12:05 rbren

Yeah my bad. Just fixed the error, plus fixing the frontend screenshot not showing up. @rbren

frankxu2004 avatar May 06 '24 17:05 frankxu2004

Thanks Frank!

rbren avatar May 06 '24 17:05 rbren