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

Make FiniteDifferenceHvp pickleable #745

Merged
merged 2 commits into from Jun 25, 2019
Merged

Make FiniteDifferenceHvp pickleable #745

merged 2 commits into from Jun 25, 2019

Conversation

ahtsan
Copy link
Contributor

@ahtsan ahtsan commented Jun 23, 2019

Currently all policies using FiniteDifferenceHvp in examples/tf/ don't work because FiniteDifferenceHvp is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO usage.

@codecov
Copy link

codecov bot commented Jun 23, 2019

Codecov Report

Merging #745 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #745      +/-   ##
==========================================
+ Coverage   67.57%   67.59%   +0.01%     
==========================================
  Files         164      164              
  Lines       10197    10201       +4     
  Branches     1335     1335              
==========================================
+ Hits         6891     6895       +4     
  Misses       2958     2958              
  Partials      348      348
Impacted Files Coverage Δ
...rage/tf/optimizers/conjugate_gradient_optimizer.py 81.18% <100%> (+0.41%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96342cd...dd0df9b. Read the comment docs.

@ryanjulian
Copy link
Member

@ahtsan can you add a test which would have caught this bug? i think you can just run one step of TRPO with the LSTM policy.

Copy link
Member

@ryanjulian ryanjulian left a comment

Choose a reason for hiding this comment

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

please add a test which would have caught this

del new_dict['opt_fun']
return new_dict

def __setstate__(self, state):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to re-create opt_fun here?
If you don't, then you should be able to delete this __setstate__ implementation, since this is the "default."
Note that you'll still have a __getstate__ implementation then, despite not having a __setstate__ implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, will remove __getstate__ since opt_fun is recreated somewhere else.

Copy link
Member

@zhanpenghe zhanpenghe left a comment

Choose a reason for hiding this comment

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

LGTM as long as you resolve Ryan and KR's comments

@ryanjulian ryanjulian merged commit 36a7491 into master Jun 25, 2019
@ryanjulian ryanjulian deleted the fix_pickle_finite branch June 25, 2019 18:22
@ryanjulian ryanjulian added this to the v2019.06 milestone Jun 25, 2019
krzentner pushed a commit that referenced this pull request Jun 27, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
gitanshu pushed a commit that referenced this pull request Jun 27, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
gitanshu pushed a commit that referenced this pull request Jun 27, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
gitanshu pushed a commit that referenced this pull request Jun 28, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
avnishn pushed a commit that referenced this pull request Jun 28, 2019
* Make FiniteDifferenceHvp pickleable (#745)

Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.

* Convert tests to run with pytest

* Use pytest-cov for coverage

* Fix broken tests

* Fix code formatting

* Fix more formatting

* Fix typo in TestSanpshotter class name

* Update new tests to use pytest format
ahtsan added a commit that referenced this pull request Jun 30, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
zequnyu pushed a commit that referenced this pull request Jul 1, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
ahtsan added a commit that referenced this pull request Jul 2, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
ahtsan added a commit that referenced this pull request Jul 2, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
nish21 pushed a commit that referenced this pull request Jul 3, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
ahtsan added a commit that referenced this pull request Jul 3, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
nish21 pushed a commit that referenced this pull request Jul 4, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
ahtsan added a commit that referenced this pull request Jul 5, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
nish21 pushed a commit that referenced this pull request Jul 6, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
nish21 pushed a commit that referenced this pull request Jul 11, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
nish21 pushed a commit that referenced this pull request Jul 23, 2019
Currently all policies using `FiniteDifferenceHvp` in `examples/tf/`
don't work because `FiniteDifferenceHvp` is not pickleable.

This PR fixes the issue, and also update outdated arguments in some TRPO
usage.
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.

None yet

5 participants