Skip to content
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

CLI documentation #743

Merged
merged 10 commits into from
Jul 14, 2023
Merged

CLI documentation #743

merged 10 commits into from
Jul 14, 2023

Conversation

ernestum
Copy link
Collaborator

@ernestum ernestum commented Jul 4, 2023

Description

Add some documentation for the command line interface.

Testing

Documentation only changes.

@ernestum ernestum linked an issue Jul 4, 2023 that may be closed by this pull request
7 tasks
@ernestum ernestum added this to the Release v0.4.0 milestone Jul 4, 2023
@RedTachyon
Copy link
Contributor

One thing worth mentioning (discussed yesterday with Adam) is that if an environment is registered upon importing a package, or in a certain file (e.g. inside custom_env.py you call gym.register("CustomEnv-v0")), then in the CLI interface you can train with that environment by providing the package/file name in the env ID (e.g. custom_env:CustomEnv-v0)

Some information about this functionality is in the Gymnasium docs. The same thing worked in gym==0.21.0 as well

@ernestum
Copy link
Collaborator Author

ernestum commented Jul 5, 2023

Thanks for letting me know! This will also benefit the rl-baselines3-zoo!

I will add an example with this to the documentation.

Note: depends on huggingface/huggingface_sb3#32 to go upstream because environment names with the package prefix are not properly normalized as of now.

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #743 (32582eb) into master (b452384) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #743   +/-   ##
=======================================
  Coverage   96.33%   96.33%           
=======================================
  Files          93       93           
  Lines        8789     8789           
=======================================
  Hits         8467     8467           
  Misses        322      322           
Impacted Files Coverage Δ
src/imitation/scripts/ingredients/bc.py 92.59% <ø> (ø)
...rc/imitation/scripts/ingredients/demonstrations.py 80.64% <ø> (ø)
src/imitation/scripts/ingredients/environment.py 88.88% <ø> (ø)
src/imitation/scripts/ingredients/expert.py 90.00% <ø> (ø)
src/imitation/scripts/ingredients/logging.py 87.50% <ø> (ø)
src/imitation/scripts/ingredients/policy.py 84.37% <ø> (ø)
...imitation/scripts/ingredients/policy_evaluation.py 86.95% <ø> (ø)
src/imitation/scripts/ingredients/reward.py 87.83% <ø> (ø)
src/imitation/scripts/ingredients/rl.py 90.24% <ø> (ø)
src/imitation/scripts/ingredients/wb.py 83.33% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ernestum ernestum requested a review from levmckinney July 7, 2023 15:46
Copy link
Collaborator

@levmckinney levmckinney left a comment

Choose a reason for hiding this comment

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

Looks really good. A couple of minor suggestions. Ping me for a rereview when you're ready.


Note: the cartpole environment is specified via a named configuration

.. code-block:: bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make these some kind of doc test so that they run during the build phase? I'm worried they will end up getting out of sync with the CI over time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right with this concern!

Unfortunately this is not so easy to do. See https://stackoverflow.com/questions/71497528/can-i-modify-doctest-to-test-bash-inside-sphinx-doc

We would either have to use python code and run the commands via subprocess or use some alpha-stage python module also does not quite exactly do what we need (we would have to re-write the documentation in Markdown).

Given that the CLI is going to change soon anyway, I would not put in the effort to test it right now. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The python module in question: https://github.com/jamesbehr/docshtest

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good lets just make sure to add a task to the issue tracker for the CI overall to update this part of the documentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added it to the bottom of the description here: #703

docs/getting-started/cli.rst Outdated Show resolved Hide resolved
docs/getting-started/cli.rst Outdated Show resolved Hide resolved
src/imitation/scripts/ingredients/expert.py Show resolved Hide resolved
Copy link
Collaborator

@levmckinney levmckinney left a comment

Choose a reason for hiding this comment

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

LGTM


Note: the cartpole environment is specified via a named configuration

.. code-block:: bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good lets just make sure to add a task to the issue tracker for the CI overall to update this part of the documentation.

# Conflicts:
#	src/imitation/scripts/ingredients/demonstrations.py
@ernestum ernestum merged commit 0d66184 into master Jul 14, 2023
15 checks passed
@ernestum ernestum deleted the 726-documentation-for-the-cli branch July 14, 2023 08:45
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.

Documentation for the CLI
3 participants