sotopia-lab / sotopia

Sotopia: an Open-ended Social Learning Environment (ICLR 2024 spotlight)
https://docs.sotopia.world
MIT License
164 stars 20 forks source link

[FEAT]: A better way to automatically setup redis database/ Sotopia-install #140

Open XuhuiZhou opened 4 months ago

XuhuiZhou commented 4 months ago

Description

I have been using the latest Sotopia cli and spotted some issues.

Additional Information

No response

ProKil commented 4 months ago

Could you specify the platform, installation options, previous database installation method etc?

On Wed, Jul 17, 2024 at 6:27 PM Xuhui Zhou @.***> wrote:

Assigned #140 https://github.com/sotopia-lab/sotopia/issues/140 to @ProKil https://github.com/ProKil.

— Reply to this email directly, view it on GitHub https://github.com/sotopia-lab/sotopia/issues/140#event-13544258861, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD436FQMULX6I2XZP55YIVTZM3VWXAVCNFSM6AAAAABLBPZY2GVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJTGU2DIMRVHA4DMMI . You are receiving this because you were assigned.Message ID: @.***>

XuhuiZhou commented 4 months ago

Here are the complete pytest log:

WARNING: overwriting environment variables set in the machine
overwriting variable {'REDIS_OM_URL'}
>>>PYTHON-EXEC-OUTPUT
Received JSON data in run script
Running pytest with args: ['-p', 'vscode_pytest', '--rootdir=/Users/xuhuizhou/Projects/sotopia', '/Users/xuhuizhou/Projects/sotopia/tests/cli/test_install.py::test_install']
============================= test session starts ==============================
platform darwin -- Python 3.11.2, pytest-8.2.2, pluggy-1.5.0
rootdir: /Users/xuhuizhou/Projects/sotopia
configfile: pyproject.toml
plugins: cov-5.0.0, nbmake-1.5.4, asyncio-0.23.7, anyio-4.4.0
asyncio: mode=Mode.STRICT
collected 1 item

tests/cli/test_install.py F                                              [100%]

=================================== FAILURES ===================================
_________________________________ test_install _________________________________

    def test_install() -> None:
        if platform.system() == "Darwin":
            result = runner.invoke(
                app,
                [
                    "install",
                    "--no-use-docker",
                    "--no-load-database",
                    "--overwrite-existing-data",
                ],
            )
            assert result.exit_code == 0
            time.sleep(1)
>           subprocess.run("redis-cli shutdown", shell=True, check=True)

tests/cli/test_install.py:24: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

input = None, capture_output = False, timeout = None, check = True
popenargs = ('redis-cli shutdown',), kwargs = {'shell': True}
process = <Popen: returncode: 1 args: 'redis-cli shutdown'>, stdout = None
stderr = None, retcode = 1

    def run(*popenargs,
            input=None, capture_output=False, timeout=None, check=False, **kwargs):
        """Run command with arguments and return a CompletedProcess instance.

        The returned instance will have attributes args, returncode, stdout and
        stderr. By default, stdout and stderr are not captured, and those attributes
        will be None. Pass stdout=PIPE and/or stderr=PIPE in order to capture them,
        or pass capture_output=True to capture both.

        If check is True and the exit code was non-zero, it raises a
        CalledProcessError. The CalledProcessError object will have the return code
        in the returncode attribute, and output & stderr attributes if those streams
        were captured.

        If timeout is given, and the process takes too long, a TimeoutExpired
        exception will be raised.

        There is an optional argument "input", allowing you to
        pass bytes or a string to the subprocess's stdin.  If you use this argument
        you may not also use the Popen constructor's "stdin" argument, as
        it will be used internally.

        By default, all communication is in bytes, and therefore any "input" should
        be bytes, and the stdout and stderr will be bytes. If in text mode, any
        "input" should be a string, and stdout and stderr will be strings decoded
        according to locale encoding, or by "encoding" if set. Text mode is
        triggered by setting any of text, encoding, errors or universal_newlines.

        The other arguments are the same as for the Popen constructor.
        """
        if input is not None:
            if kwargs.get('stdin') is not None:
                raise ValueError('stdin and input arguments may not both be used.')
            kwargs['stdin'] = PIPE

        if capture_output:
            if kwargs.get('stdout') is not None or kwargs.get('stderr') is not None:
                raise ValueError('stdout and stderr arguments may not be used '
                                 'with capture_output.')
            kwargs['stdout'] = PIPE
            kwargs['stderr'] = PIPE

        with Popen(*popenargs, **kwargs) as process:
            try:
                stdout, stderr = process.communicate(input, timeout=timeout)
            except TimeoutExpired as exc:
                process.kill()
                if _mswindows:
                    # Windows accumulates the output in a single blocking
                    # read() call run on child threads, with the timeout
                    # being done in a join() on those threads.  communicate()
                    # _after_ kill() is required to collect that and add it
                    # to the exception.
                    exc.stdout, exc.stderr = process.communicate()
                else:
                    # POSIX _communicate already populated the output so
                    # far into the TimeoutExpired exception.
                    process.wait()
                raise
            except:  # Including KeyboardInterrupt, communicate handled that.
                process.kill()
                # We don't call process.wait() as .__exit__ does that for us.
                raise
            retcode = process.poll()
            if check and retcode:
>               raise CalledProcessError(retcode, process.args,
                                         output=stdout, stderr=stderr)
E               subprocess.CalledProcessError: Command 'redis-cli shutdown' returned non-zero exit status 1.

../../miniconda3/envs/sotopia/lib/python3.11/subprocess.py:571: CalledProcessError
----------------------------- Captured stdout call -----------------------------
==> Fetching /opt/homebrew...

==> Resetting /opt/homebrew...

==> Fetching /opt/homebrew/Library/Taps/homebrew/homebrew-cask-fonts...

==> Resetting /opt/homebrew/Library/Taps/homebrew/homebrew-cask-fonts...
branch 'master' set up to track 'origin/master'.
Your branch is up to date with 'origin/master'.

==> Fetching /opt/homebrew/Library/Taps/redis-stack/homebrew-redis-stack...

==> Resetting /opt/homebrew/Library/Taps/redis-stack/homebrew-redis-stack...
branch 'master' set up to track 'origin/master'.
Your branch is up to date with 'origin/master'.

==> Downloading https://redismodules.s3.amazonaws.com/redis-stack/.donotremove
Already downloaded: /Users/xuhuizhou/Library/Caches/Homebrew/downloads/ef2730594e69664c1443b12b81b48a552062d289c8d16cf38e3ec96d0f8223be--.donotremove
==> Installing dependencies: redis-stack-redisinsight
==> Downloading https://s3.amazonaws.com/redisinsight.download/public/releases/2.50.0/redisstack/Redis-Insight-app-darwin.arm64.tar.gz
Already downloaded: /Users/xuhuizhou/Library/Caches/Homebrew/downloads/904434f7875a3ba3486d6ec3c5f04d1132d16b639e579a8d34e3a147cd8e2bff--Redis-Insight-app-darwin.arm64.tar.gz
==> Installing Cask redis-stack-redisinsight
==> Purging files for version 2.50.0 of Cask redis-stack-redisinsight
----------------------------- Captured stderr call -----------------------------
From https://github.com/Homebrew/brew
   9538f424b5..321498c327  master             -> origin/master
 * [new branch]            more-sorbet-strict -> origin/more-sorbet-strict
Reset branch 'stable'
Reset branch 'master'
Reset branch 'master'
Warning: Cannot verify integrity of '904434f7875a3ba3486d6ec3c5f04d1132d16b639e579a8d34e3a147cd8e2bff--Redis-Insight-app-darwin.arm64.tar.gz'.
No checksum was provided.
For your reference, the checksum is:
  sha256 "9cc8d05e3ef7af96c8801d46509a2da577f61b490b3761ab5af92ec896c53030"
Error: It seems there is already an App at '/Applications/Redis Insight.app'.
Could not connect to Redis at 127.0.0.1:6379: Connection refused
=============================== warnings summary ===============================
../../miniconda3/envs/sotopia/lib/python3.11/site-packages/beartype/_util/hint/pep/utilpeptest.py:339
  /Users/xuhuizhou/miniconda3/envs/sotopia/lib/python3.11/site-packages/beartype/_util/hint/pep/utilpeptest.py:339: BeartypeDecorHintPep585DeprecationWarning: PEP 484 type hint typing.Sequence[sotopia.agents.base_agent.BaseAgent[sotopia.messages.message_classes.Observation, sotopia.messages.message_classes.AgentAction]] deprecated by PEP 585. This hint is scheduled for removal in the first Python version released after October 5th, 2025. To resolve this, import this hint from "beartype.typing" rather than "typing". For further commentary and alternatives, see also:
      https://beartype.readthedocs.io/en/latest/api_roar/#pep-585-deprecations
    warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tests/cli/test_install.py::test_install - subprocess.CalledProcessErro...
======================== 1 failed, 1 warning in 10.89s =========================
<<<PYTHON-EXEC-OUTPUT
Finished running tests!
ProKil commented 4 months ago

It seems that the Homebrew detects another redis already installed in the system. In this case, what do you think is the desired behavior?

XuhuiZhou commented 4 months ago

in this case, maybe ask the user if they want to overwrite their current database. If not they need to do things manually, and we output some pointers?

ProKil commented 4 months ago

There are two things:

  1. They have rdb file at the location that we want to copy to.
  2. They have installed redis with homebrew before.

For 1, we already have that. For 2, I don't know:

  1. how to know if this happens
  2. what are the options for users?
ProKil commented 3 months ago

It seems that this is expected? What is the requested API change here?