Integrate VirtualEnv management into Component

[nickjalbert]

As the title says, this PR moves venv management into the Component. It also cleans up the VirtualEnv API a little bit (will create an additional issue for further improvements see #297):

The following demo now works (note, no manual venv management required). First in bash, create a new empty Python environment to run the demo script and install agentos:

virtualenv -p /usr/bin/python3.9 test_env
source test_env/bin/activate
pip install -e .

Now try to manually build and run the sb3_agent in the Python REPL:

from agentos import Repo, Component

aos_repo = Repo.from_github("agentos-project", "agentos")

sb3_agent = Component.from_repo(repo=aos_repo, identifier="sb3_agent==master", class_name="SB3PPOAgent", file_path="example_agents/sb3_agent/agent.py", requirements_path="example_agents/sb3_agent/requirements.txt")
env = Component.from_repo(repo=aos_repo, identifier="environment==master", class_name="CartPole", file_path="example_agents/sb3_agent/environment.py")
run = Component.from_repo(repo=aos_repo, identifier="SB3AgentRun==master", class_name="SB3Run", file_path="example_agents/sb3_agent/sb3_run.py", instantiate=False)

sb3_agent.add_dependency(env, attribute_name="environment")
sb3_agent.add_dependency(run, attribute_name="SB3AgentRun")
sb3_agent.run("evaluate")

Fixes #280.

[andyk]

LGTM. I asked a couple of questions about bigger picture architecture and future plans. I'm fine with merging it as-is, unless you think any changes make sense given my comments.

[andyk]

Why can't we setup environment handling as part of the Component creation (and thus Virtual Env creation) process? I.e., in this case that would happen as part of the Component.from_registry_file() call on line 235.

[nickjalbert]

Hmm, yeah, that seems better.

One (related) awkwardness is that every time we add a new argument to Component.__init__(), we (usually) need to add that argument to all the Component.from_*() methods, even if it's just passed through to the Component constructor.

I'm thinking there might be a way to streamline this when we get to the point of making the Component constructor the one source of truth for the spec structure.

But for now, it does seem nicer to add an optional argument to the Component.from_*() methods instead of a separate setter/getter. Will fix!

[andyk]

It feels a little confusing to return a VirtualEnv where everything is a no-op. IIRC, we do this because the no-op venv is equivalent to using the existing the developer's env (which may be a venv or may not), and it is cleaner to just have a no-op VirtualEnv that encapsulates all of the if use_venv: ... statements than having to use those if statements wherever we use the VirtualEnv instance. Is that right?

If I'm right, then it seems like returning a NoOpVirtualEnv or DummyVirtualEnv that implements the same interface as VirtualEnv might be more intuitive than having a VirtualEnv.use_venv attribute.

But I'm very likely missing other constraint or nuance here!

Maybe you're planning on cleaning this up as part of #297?

If we do keep the no-op behavior here in VirtualEnv as is, then can we document it in the VirtualEnv class docstring?

[nickjalbert]

it is cleaner to just have a no-op VirtualEnv that encapsulates all of the if use_venv: ... statements than having to use those if statements wherever we use the VirtualEnv instance. Is that right?

Yep, you got it. I have found myself wanting the following pattern several times in the codebase:

if flag:
    with context_manager():
        do_thing()
else:
    do_thing()

but that feels like an awkward pattern to me, so I often try to squish it into straight-line code that always gets a context manager (but that context manager may do nothing, depending on flag):

with get_maybe_noop_context_manager(flag):
    do_thing()

Maybe the latter is less grokkable than the former approach; I'm not sure. Happy to standardize on either one if you have a strong opinion.

But yeah, I like the idea of a NoOpVirtualEnv. Will fix!

[nickjalbert]

Alrighty @andyk, addressed your comments. Will merge when green.

Thanks, as always, for the review! :)