Skip to content

Remove awscli dependency#572

Merged
dfilan merged 11 commits into
masterfrom
remove-awscli
Oct 12, 2022
Merged

Remove awscli dependency#572
dfilan merged 11 commits into
masterfrom
remove-awscli

Conversation

@dfilan

@dfilan dfilan commented Oct 6, 2022

Copy link
Copy Markdown
Collaborator

Description

Fixes #560.

Testing

Ran all tests. Results: 3543 passed, 595 skipped, 4234 warnings.

@codecov

codecov Bot commented Oct 6, 2022

Copy link
Copy Markdown

Codecov Report

Merging #572 (ac944e3) into master (005c15f) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #572      +/-   ##
==========================================
- Coverage   97.43%   97.42%   -0.02%     
==========================================
  Files          88       88              
  Lines        8375     8378       +3     
==========================================
+ Hits         8160     8162       +2     
- Misses        215      216       +1     
Impacted Files Coverage Δ
tests/test_experiments.py 100.00% <ø> (ø)
src/imitation/scripts/config/eval_policy.py 76.81% <66.66%> (-0.47%) ⬇️

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

@dfilan

dfilan commented Oct 6, 2022

Copy link
Copy Markdown
Collaborator Author

TODO get rid of scripts that download from aws

@dfilan

dfilan commented Oct 10, 2022

Copy link
Copy Markdown
Collaborator Author

Having a touch of trouble testing due to #576.

@dfilan

dfilan commented Oct 10, 2022

Copy link
Copy Markdown
Collaborator Author

Current status:

  • have removed experiment scripts that critically rely on aws
  • have retouched experiments/README.md accordingly
  • have edited experiment config files to match with what's in HuggingFace
  • experiment scripts don't currently work, because they don't like the format in which things are saved in HuggingFace

I turned the last bullet point into its own issue (#576), but think it should be addressed separately - IMO it kind of sucks that someone can't just clone this repository and run pip install .[dev] but instead has to comment out that bit of setup.py.

@dfilan
dfilan requested a review from AdamGleave October 10, 2022 14:18
@dfilan

dfilan commented Oct 10, 2022

Copy link
Copy Markdown
Collaborator Author

BTW I kept imit_benchmark.sh because I think it makes sense as its own thing, even without the context of download_models.sh.

@AdamGleave AdamGleave left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM apart from deleting one section that I think shouldn't have been.

Comment thread experiments/README.md

To analyze these results, run `python -m imitation.scripts.analyze with run_name=RUN_NAME`. Analysis can be run even while training is midway (will only show completed imitation learner's results). [Example output.](https://gist.github.com/shwang/4049cd4fb5cab72f2eeb7f3d15a7ab47)

### Phase 4: Transfer learning.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did you delete this? If it was because experiments/transfer_benchmark.sh doesn't exist I think it just got renamed to experiments/transfer_learn_benchmark.sh.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think maybe I deleted it because currently stuff doesn't save to data/reward_models. I guess I can reinstate it and leave fixing it for a general "make the experiment scripts work" PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Have reinstated it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah yeah that is a problem but agree probably best to punt it to another issue. We should really overhaul the whole experiments/ directory -- I expect @taufeeque9 will end up doing this as part of his benchmarking work.

@dfilan
dfilan merged commit 41c41b1 into master Oct 12, 2022
@dfilan
dfilan deleted the remove-awscli branch October 12, 2022 10:01
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.

pip install -e .[dev] installs tons of versions of awscli

2 participants