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

rllab ec2 tutorial #40

Merged
merged 45 commits into from
Dec 4, 2018
Merged

rllab ec2 tutorial #40

merged 45 commits into from
Dec 4, 2018

Conversation

nskh
Copy link
Collaborator

@nskh nskh commented Aug 25, 2018

draft tutorial is in this commit. let me know what you think.

also - an idea: these shouldn't be tutorials, but should go into our documentation. they can be easily converted to .rst files and would look nice in our docs. thoughts?

(a copy of berkeleyflow/flow#70)
(how would i remove the number of commits here?)

eugenevinitsky and others added 30 commits August 9, 2018 15:29
Extremely minor change to have the tests run faster. Backoff is not applied if we are in unit test mode.
@nskh nskh requested a review from kanaadp as a code owner August 25, 2018 01:35
@AboudyKreidieh
Copy link
Member

don't worry, commits are fine (they reflect our actual commit history). Two comments though:

  1. I think some stuff are left behind that should be there (e.g. the warnings folder). Can you please delete them?
  2. You mentioned turning this into a document in readthedocs instead. Do you wanna do that for this PR?

@nskh nskh changed the title rllab ec2 rllab ec2 tutorial Aug 25, 2018
@nskh
Copy link
Collaborator Author

nskh commented Aug 25, 2018

@AboudyKreidieh - deleted the old files and added a page to the documentation. Not 100% where to put this tutorial... we might need some sort of subfolder structure for our docs, in which case we could put Setup Instructions, Tutorials, and the rllib+rllab tutorials inside those.

@eugenevinitsky, thoughts?

Both of you, let me know what you think of this form of the tutorial. make html inside docs to build the new docs and check it out inside docs/build/html/. FYI, the notebook tutorial still exists in this commit.

@eugenevinitsky
Copy link
Member

Hey nish, can you add the MakeFile to this?

@eugenevinitsky
Copy link
Member

Want to review it but I can't make it.

@nskh
Copy link
Collaborator Author

nskh commented Aug 31, 2018

@eugenevinitsky the Makefile should already be there - are you running make html from inside docs/?

@eugenevinitsky
Copy link
Member

Huh I guess it is, looking at this PR again

kjang96 pushed a commit to kjang96/flow-1 that referenced this pull request Sep 1, 2018
First, follow the `rllab cluster setup
instructions <https://rllab.readthedocs.io/en/latest/user/cluster.html>`__
with region ``us-west-1``. Modify ``rllab/config_personal.py`` to
reference the most current Flow Docker image.
Copy link
Member

Choose a reason for hiding this comment

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

can we add the name of the most current Flow docker image? @kjang96 which one are you using?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fywu85 @kjang96 @lucfisc any updates on the docker image to reference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fywu85/flow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fywu85 @lucfisc @kjang96 we should figure out what docker image to mention here

Navigate to your ``flow`` directory and modify ``Makefile.template`` per
the instructions in that file. The variable ``RLLABDIR`` should be the
relative path from your ``flow`` directory to ``rllab`` and should not
have a backslash at the end.
Copy link
Member

Choose a reason for hiding this comment

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

Bold "should not have a backslash at the end"

- Ensure you have committed or otherwise tracked the state of your
``flow`` directory, because that instance is what will be used to run
your experiment. Upon visualization, the same files will need to be
used—for example, changes to environment's state-space would break
Copy link
Member

Choose a reason for hiding this comment

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

"changes to your environment's state-space"

your experiment. Upon visualization, the same files will need to be
used—for example, changes to environment's state-space would break
the ability to run a trained policy using a different state space.
Check out an old commit of your ``flow`` directory and run
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe you need to run make prepare to use the visualization tools?"


Inside the experiment, change the ``mode`` to ``ec2`` (other options are
``local`` and ``local_docker``). You should run the experiment in
``local_docker`` mode briefly before running the ``ec2`` version to
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a descripton of what running in local_docker tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you think I should say here?

Copy link
Member

@eugenevinitsky eugenevinitsky 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! Thanks for doing this Nish.

@eugenevinitsky
Copy link
Member

@nskh I like the idea of a tutorials substructure in the docs, can you create one and add this to it? I'm making an rllib tutorial and will include it there as well.

@nskh
Copy link
Collaborator Author

nskh commented Sep 9, 2018

@eugenevinitsky updated this but have some questions before merging - let me know when you see this.

@coveralls
Copy link

coveralls commented Dec 4, 2018

Pull Request Test Coverage Report for Build 1821

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 84.355%

Totals Coverage Status
Change from base Build 1791: 0.05%
Covered Lines: 6686
Relevant Lines: 7926

💛 - Coveralls

@nskh nskh merged commit 0ee8ce9 into flow-project:master Dec 4, 2018
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.

5 participants