Added main entry point #109
Conversation
whitead
requested review from
sidnarayanan,
Ryan-Rhys,
albertbou92 and
jamesbraza
ldp/alg/callbacks.py
Outdated
| @@ -475,3 +475,53 @@ async def after_eval_loop(self) -> None: | |||
| await super().after_eval_loop() # Call the parent to compute means | |||
| if self.eval_means: | |||
| self._log_filtered_metrics(self.eval_means, step_type="Eval") | |||
|
|
|||
|
|
|||
| class TerminalLoggingCallback(Callback): | |||
There was a problem hiding this comment.
Feel free to ignore for this PR, but we should refactor our logging callback to be general, and move this terminal logging callback to be a composition of the logging callback with a stdout handler or a RichHandler
Our callbacks are a point of tech debt in this framework imo
There was a problem hiding this comment.
Think they do different things though. The one I made is just printing the action actions/observations. The logging callback is a MeanMetricsCallback that logs metrics to logs. Feel like they're pretty different. Maybe we need a guide or something for logging or callbacks :)
| from rich.pretty import pprint # noqa: F401 | ||
| except ImportError: | ||
| raise ImportError( | ||
| f"rich is required for {type(self).__name__}. Please install it with `pip install rich`." |
There was a problem hiding this comment.
We should make a rich-specific extra and move our current rich extra to be rich-progress
Or we can leave as is
| print("\nAction:") | ||
| pprint(action.value, expand_all=True) |
There was a problem hiding this comment.
Why have 2+ print invocations? Can we make it just one with \n?
There was a problem hiding this comment.
These are different - print vs pprint?
There was a problem hiding this comment.
Yeah why not just always use pprint?
There was a problem hiding this comment.
How would you concat them?
pprint("Action\n", action.value) is not valid and pprint(f"Action\n{str(action.value)}") defeats the purpose of using pprint
and pprint("Action\n") prints the python rep of that string instead of literally printing the words Action followed by a newline.
There was a problem hiding this comment.
Ah I see, you're right. Thanks for going through this with me 👍
ldp/alg/callbacks.py
Outdated
| except ImportError: | ||
| raise ImportError( | ||
| f"rich is required for {type(self).__name__}. Please install it with `pip install rich`." | ||
| ) |
There was a problem hiding this comment.
Why not raise ImportError("...") from exc? Maybe it's me, but I find having the "niceified" error connected to the un-niceified error to be useful
ldp/main.py
Outdated
| from ldp.alg.rollout import RolloutManager | ||
|
|
||
|
|
||
| def agent_factory(agent: Agent | str | PathLike) -> Agent: |
There was a problem hiding this comment.
Normally a factory is like a classmethod going from a type to an instance. Can we have this support type[Agent]?
Also, can you add a docstring stating the possible modes checked?
"""
Construct an agent.
Args:
agent: Either an agent instance, a name for an agent, or a path to an agent pickle.
"""There was a problem hiding this comment.
This is more logically connected to parsing user input - I extracted it out of main - and so I think it's better here where I use it.
I changed the name. Feel like the type hints and name are enough documentation.
There was a problem hiding this comment.
What do you think of naming this resolve_agent? Also, feel free to ignore this one
pyproject.toml
Outdated
| @@ -317,6 +317,7 @@ ignore = [ | |||
| "ARG003", # Thrown all the time when we are subclassing | |||
| "ASYNC109", # Buggy, SEE: https://github.com/astral-sh/ruff/issues/12353 | |||
| "ASYNC2", # It's ok to mix async and sync ops (like opening a file) | |||
| "B904", | |||
There was a problem hiding this comment.
I like B904 because it makes the code explicit, but I am not that attached to it.
Would you be open to reverting this? I don't think this PR uses it
ldp/main.py
Outdated
| from ldp.alg.rollout import RolloutManager | ||
|
|
||
|
|
||
| def agent_factory(agent: Agent | str | PathLike) -> Agent: |
There was a problem hiding this comment.
What do you think of naming this resolve_agent? Also, feel free to ignore this one
| path = Path(agent) | ||
| if not path.exists(): | ||
| raise ValueError(f"Could not resolve agent: {agent}") | ||
|
|
||
| with path.open("rb") as f: | ||
| return pickle.load(f) # noqa: S301 |
There was a problem hiding this comment.
| path = Path(agent) | |
| if not path.exists(): | |
| raise ValueError(f"Could not resolve agent: {agent}") | |
| with path.open("rb") as f: | |
| return pickle.load(f) # noqa: S301 | |
| try: | |
| with path.open("rb") as f: | |
| return pickle.load(f) # noqa: S301 | |
| except FileNotFoundError: | |
| raise ValueError(f"Could not resolve agent: {agent}") from None |
Alternate way of doing this
| print("\nAction:") | ||
| pprint(action.value, expand_all=True) |
There was a problem hiding this comment.
Ah I see, you're right. Thanks for going through this with me 👍
This is a basic entry point for running agents with an environment for tasks not derived from the task library. Depends on Future-House/aviary#74