Dev: rebase master #150

youkaichao · 2020-07-20T00:37:02Z

I have marked all applicable categories:
- exception-raising fix
- algorithm implementation fix
- documentation modification
- new feature
If applicable, I have mentioned the relevant/related issue(s)

Less important but also useful:

I have visited the source website, and in particular read the known issues
I have searched through the issue tracker and issue categories for duplicates

I have mentioned version numbers, operating system and environment, where applicable:

import tianshou, torch, sys
print(tianshou.__version__, torch.__version__, sys.version, sys.platform)

* make sure the key type of Batch is string, and add unit tests * add is_empty() function and unit tests * enable cat of mixing dict and Batch, just like stack

* minor polish * improve and implement Batch.cat_ * bugfix for buffer.sample with field impt_weight * restore the usage of a.cat_(b) * fix 2 bugs in batch and add corresponding unittest * code fix for update * update is_empty to recognize empty over empty; bugfix for len * bugfix for update and add testcase * add testcase of update * fix docs * fix docs * fix docs [ci skip] * fix docs [ci skip] Co-authored-by: Trinkle23897 <463003665@qq.com>

…ml#130) * re-implement Batch.stack and add testcases * add doc for Batch.stack * reuse _create_values and refactor stack_ & cat_ * fix pep8 * fix docs * raise exception for stacking with partial keys and axis!=0 * minor fix * minor fix Co-authored-by: Trinkle23897 <463003665@qq.com>

* remove multibuf * reward_metric * make fileds with empty Batch rather than None after reset * many fixes and refactor Co-authored-by: Trinkle23897 <463003665@qq.com>

* Enable selecting worker for vector env step method. * Update collector to match new vecenv selective worker behavior. * Bug fix. * Fix rebase Co-authored-by: Alexis Duburcq <alexis.duburcq@wandercraft.eu>

* code refactor; remove unused kwargs; add reward_normalization for dqn * bugfix for __setitem__ with torch.Tensor; add Batch.condense * minor fix * support cat with empty Batch * remove the dependency of is_empty on len; specify the semantic of empty Batch by test cases * support stack with empty Batch * remove condense * refactor code to reflect the shared / partial / reserved categories of keys * add is_empty(recursive=False) * doc fix * docfix and bugfix for _is_batch_set * add doc for key reservation * bugfix for algebra operators * fix cat with lens hint * code refactor * bugfix for storing None * use ValueError instead of exception * hide lens away from users * add comment for __cat * move the computation of the initial value of lens in cat_ itself. * change the place of doc string * doc fix for Batch doc string * change recursive to recurse * doc string fix * minor fix for batch doc

* add doc for len exceptions * doc move; unify is_scalar_value function * remove some issubclass check * bugfix for shape of Batch(a=1) * keep moving doc * keep writing batch tutorial * draft version of Batch tutorial done * improving doc * keep improving doc * batch tutorial done * rename _is_number * rename _is_scalar * shape property do not raise exception * restore some doc string * grammarly [ci skip] * grammarly + fix warning of building docs * polish docs * trim and re-arrange batch tutorial * go straight to the point * minor fix for batch doc * add shape / len in basic usage * keep improving tutorial * unify _to_array_with_correct_type to remove duplicate code * delegate type convertion to Batch.__init__ * further delegate type convertion to Batch.__init__ * bugfix for setattr * add a _parse_value function * remove dummy function call * polish docs Co-authored-by: Trinkle23897 <463003665@qq.com>

* Enable selecting worker for vector env step method. * Update collector to match new vecenv selective worker behavior. * Bug fix. * Fix rebase Co-authored-by: Alexis Duburcq <alexis.duburcq@wandercraft.eu>

* code refactor; remove unused kwargs; add reward_normalization for dqn * bugfix for __setitem__ with torch.Tensor; add Batch.condense * minor fix * support cat with empty Batch * remove the dependency of is_empty on len; specify the semantic of empty Batch by test cases * support stack with empty Batch * remove condense * refactor code to reflect the shared / partial / reserved categories of keys * add is_empty(recursive=False) * doc fix * docfix and bugfix for _is_batch_set * add doc for key reservation * bugfix for algebra operators * fix cat with lens hint * code refactor * bugfix for storing None * use ValueError instead of exception * hide lens away from users * add comment for __cat * move the computation of the initial value of lens in cat_ itself. * change the place of doc string * doc fix for Batch doc string * change recursive to recurse * doc string fix * minor fix for batch doc

* add doc for len exceptions * doc move; unify is_scalar_value function * remove some issubclass check * bugfix for shape of Batch(a=1) * keep moving doc * keep writing batch tutorial * draft version of Batch tutorial done * improving doc * keep improving doc * batch tutorial done * rename _is_number * rename _is_scalar * shape property do not raise exception * restore some doc string * grammarly [ci skip] * grammarly + fix warning of building docs * polish docs * trim and re-arrange batch tutorial * go straight to the point * minor fix for batch doc * add shape / len in basic usage * keep improving tutorial * unify _to_array_with_correct_type to remove duplicate code * delegate type convertion to Batch.__init__ * further delegate type convertion to Batch.__init__ * bugfix for setattr * add a _parse_value function * remove dummy function call * polish docs Co-authored-by: Trinkle23897 <463003665@qq.com>

Trinkle23897 · 2020-07-20T05:26:17Z

I have no idea how to solve the conflict. Since in #133 I use "rebase and merge", and GitHub uses its customized "rebase" command and changes the commit hash. I tried #148 and Kaichao tried #149 and this commit but we are not satisfied with the result.

I think there are two ways to solve it:

force push to master to revert the commit hash to be the same as dev. Need to re-tag 0.2.4.post1.
merge master to dev (as this PR), but I think it may cause some other issues, for example, other PR based on old dev needs to resolve the conflict.

Maybe there is a third and better way?

And is it a must for the current dev branch? I look at other awesome repos on GitHub and they all use master instead of dev.

youkaichao · 2020-07-20T05:50:05Z

merge master to dev (as this PR), but I think it may cause some other issues, for example, other PR based on old dev needs to resolve the conflict.

I tried in my local environment. Many conflicts.

force push to master to revert the commit hash to be the same as dev. Need to re-tag 0.2.4.post1.

I vote for this. Since dev is our main branch and master is only used for release, it seems we can reset the head of master to the head of dev to avoid these annoying issues.

duburcqa · 2020-07-20T06:17:03Z

force push to master to revert the commit hash to be the same as dev. Need to re-tag 0.2.4.post1.

I would rather avoid forcing master branch at all cost. Personally I force push dev branch instead after every merge on master. For now, just do a proper rebase --onto while rebasing dev on master to get only the new commits since the last release, force push dev, then merge dev on master, then force push dev` on new master. That's it.

youkaichao · 2020-07-20T06:43:10Z

I’m lost.

For now, just do a proper rebase --onto while rebasing dev on master to get only the new commits since the last release

The last release is v0.2.4.post1, and is the head of master now. So git rebase master is the same as git rebase master --onto v0.2.4.post1.

We can do this on dev, but I think what @Trinkle23897 concerns is that this forced push in dev will make pull requests like #122 have many conflicts.

duburcqa · 2020-07-20T07:10:50Z

The last release is v0.2.4.post1, and is the head of master now. So git rebase master is the same as git rebase master --onto v0.2.4.post1.

Absolutely not.

duburcqa · 2020-07-20T07:12:57Z

git rebase --onto upstream/master a55ad33 upstream/dev

And here you go ! Pretty easy, isn't it ?

The, force push the new upstream/dev branch, and your are good to go merging it on master. After that, do not forget to push force dev on master this time 😄

Trinkle23897 · 2020-07-20T07:31:18Z

I vote for this. Since dev is our main branch and master is only used for release, it seems we can reset the head of master to the head of dev to avoid these annoying issues.

I don't like the way to treat master as a release-only branch. I found few repo do like this. The separation of master and dev has caused some extra problems for us in my opinion. So I'm here discussing this issue.

Trinkle23897 · 2020-07-20T07:33:27Z

git rebase --onto upstream/master a55ad33 upstream/dev
And here you go ! Pretty easy, isn't it ?

The, force push the new upstream/dev branch, and your are good to go merging it on master. After that, do not forget to push force dev on master this time

I'll try it but I don't know whether this will cause some extra conflicts with current unmerged PRs.
Seems like all is well.

duburcqa · 2020-07-20T07:47:03Z

I don't like the way to treat master as a release-only branch. I found few repo do like this.

Same here. dev should be use to increase flexibility and fast release of unstable PR. So far, we are not really using it this way.

The separation of master and dev has caused some extra problems for us in my opinion. So I'm here discussing this issue.

I don't think so. We just need to get use of it. It is pretty easy to manage if done carefully.

duburcqa · 2020-07-20T07:48:42Z

I'll try it but I don't know whether this will cause some extra conflicts with current unmerged PRs.
Seems like all is well.

No it should not. And if it is, it can always be fixed with a simple rebase --onto. So no big deal.

Trinkle23897 · 2020-07-20T07:52:14Z

I'll try it but I don't know whether this will cause some extra conflicts with current unmerged PRs.
Seems like all is well.

No it should not. And if it is, it can always be fixed with a simple rebase --onto. So no big deal.

I tested and except for #143, other PRs have conflict. But I think these are no big deal too.

duburcqa · 2020-07-20T08:02:19Z

I'm done with the rebase on my PRs.

Trinkle23897 · 2020-07-20T08:15:32Z

I still want to discuss this thing:
https://softwareengineering.stackexchange.com/a/312024

Having worked with a develop branch for a year or two, just to have it merged into master every now and then, I can just say: amen, abandon that thing :]

I think the first issue is that we cannot perform a documentation fix or a bug fix in time by using the dev branch.

If we merge the unstable PR into master instead of dev, users can checkout the tag like "0.2.4.post1" in git and readthedocs for both for the stable version.

Trinkle23897 · 2020-07-20T12:58:32Z

I still want to discuss this thing:
https://softwareengineering.stackexchange.com/a/312024

Having worked with a develop branch for a year or two, just to have it merged into master every now and then, I can just say: amen, abandon that thing :]

I think the first issue is that we cannot perform a documentation fix or a bug fix in time by using the dev branch.

If we merge the unstable PR into master instead of dev, users can checkout the tag like "0.2.4.post1" in git and readthedocs for both for the stable version.

I withdraw what I have said above. Current test_drqn fails on cuda device, and if it appears in the master, some users may get annoyed. This is more important.

duburcqa · 2020-07-20T13:23:07Z

And anyway, it is impossible to rely only on unit tests to make sure everything is working properly. Having new features available on dev branch first during a few weeks is a good way to ensure that everything gets heavily tested before releasing them on master. Patching master must be avoided at all cost for credible projects.

Trinkle23897 · 2020-07-20T13:34:12Z

I think my issue can be solved at https://tianshou.readthedocs.io/en/dev/

duburcqa · 2020-07-20T13:43:44Z

I think my issue can be solved at https://tianshou.readthedocs.io/en/dev/

Awesome ! For me it is the way to go without any doubt.

youkaichao and others added 10 commits July 11, 2020 09:44

Improve Batch (thu-ml#126)

e976d74

* make sure the key type of Batch is string, and add unit tests * add is_empty() function and unit tests * enable cat of mixing dict and Batch, just like stack

Improve collector (thu-ml#125)

885fbc1

* remove multibuf * reward_metric * make fileds with empty Batch rather than None after reset * many fixes and refactor Co-authored-by: Trinkle23897 <463003665@qq.com>

Vector env enable select worker (thu-ml#132)

cee8088

* Enable selecting worker for vector env step method. * Update collector to match new vecenv selective worker behavior. * Bug fix. * Fix rebase Co-authored-by: Alexis Duburcq <alexis.duburcq@wandercraft.eu>

Vector env enable select worker (thu-ml#132)

c198c60

* Enable selecting worker for vector env step method. * Update collector to match new vecenv selective worker behavior. * Bug fix. * Fix rebase Co-authored-by: Alexis Duburcq <alexis.duburcq@wandercraft.eu>

Merge branch 'dev' into dev

977b627

Trinkle23897 force-pushed the dev branch from fa542f8 to fe5555d Compare July 20, 2020 07:54

Trinkle23897 closed this Jul 20, 2020

Dev: rebase master #150

Dev: rebase master #150

Uh oh!

Conversation

youkaichao commented Jul 20, 2020

Uh oh!

Trinkle23897 commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youkaichao commented Jul 20, 2020

Uh oh!

duburcqa commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youkaichao commented Jul 20, 2020

Uh oh!

duburcqa commented Jul 20, 2020

Uh oh!

duburcqa commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trinkle23897 commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trinkle23897 commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duburcqa commented Jul 20, 2020

Uh oh!

duburcqa commented Jul 20, 2020

Uh oh!

Trinkle23897 commented Jul 20, 2020

Uh oh!

duburcqa commented Jul 20, 2020

Uh oh!

Trinkle23897 commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trinkle23897 commented Jul 20, 2020

Uh oh!

duburcqa commented Jul 20, 2020

Uh oh!

Trinkle23897 commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duburcqa commented Jul 20, 2020

Uh oh!

Uh oh!

Trinkle23897 commented Jul 20, 2020 •

edited

Loading

duburcqa commented Jul 20, 2020 •

edited

Loading

duburcqa commented Jul 20, 2020 •

edited

Loading

Trinkle23897 commented Jul 20, 2020 •

edited

Loading

Trinkle23897 commented Jul 20, 2020 •

edited

Loading

Trinkle23897 commented Jul 20, 2020 •

edited

Loading

Trinkle23897 commented Jul 20, 2020 •

edited

Loading