[Fetch]Fetchに関するPRの出し方について

[sktometometo]

現在、knorth55/fetch15 を開発ブランチとしてjsk_fetch_robot以下の開発が行われていますが、jsk-rospkg/masterへpull request を出す際に以下の問題があります。

• knorth55/fetch15 のどの commit が pull requests に出されているのかがわかりづらい
• 新規に jsk-ros-pkg/master へ pull request を作成する際に何に依存していると明記するべきかわかりづらい
• pull request間の依存関係がわかりづらいため、特定のpull request がマージされるとconflictが発生する

これを解決したいです。

• まずpull requestに出されているcommitとまだ出されていないcommitを管理するために, 現在の jsk-ros-pkg/master と jsk-ros-pkg/jsk_robot に出されている fetch関係の pull requests をマージしたブランチ master-with-pull-requests を作成しました。
• 新規に pull request を作成する際のワークフローを策定する必要があると考えます。

master-with-pull-requests ブランチ

現在、この master_with_pull_requests ブランチには以下のPRがマージされています。

• [fetcheus] add fetch head box collision #1186
• [jsk_fetch_startup] add fetch etc files #1208
• [jsk_robot_startup, jsk_fetch_startup] add rwt_image_view launch for fetch #1225
• [jsk_fetch_startup] Shutdown robot if go-to-kitchen timeout or fail #1453
• Improve fetch's move base by fetchrobotics's default params #1456
• Check if the ROS topics using the fetch device are published. #1460
• Add supervisor job for edgetpu-panorama recognition #1465
• [jsk_fetch_startup] Use audible_warning to speak diagnostics #1480
• [jsk_fetch_startup] Publish diagnostics for important topics and nodes #1481

以下のPRは jsk-ros-pkg/master と conflict があるためマージしていません。

• Add supervisor scripts #1323
• Add cron install scripts #1332
• Show logs by tmuxinator log #1337
• [jsk_fetch_startup] add fetch light on and off app #1352
• [jsk_fetch_startup] Update charge level warning to prevent battery degradation #1422

以下のPRは互いに干渉して conflicts を起こすためマージしていません。

• Fetch notifies trashbox status in go-to-kitchen demo. #1458 とマージされている何か
  • [jsk_fetch_startup] Update trashcan front position in kitchen demo #1462 がこれに依存している
  • [jsk_fetch_startup] Enable to attach pictures to kitchen demo #1463 がさらにこれに依存している
• [Waiting for upstream] Use VOICEVOX by default #1466 とマージされている何か

この上で Pull Request を出す際のワークフローについて議論したいです。
個人的には手戻りを起こす&発生する可能性があるpull requestが現在のように大量に出ている状態で jsk-ros-pkg/master に pull requestを出す気はあまりないです。

[sktometometo]

@knorth55 @708yamaguchi @mqcmd196 @tkmtnt7000 @k-okada

[k-okada]

マージされていないのは @k-okada 不作為というのもありますが

1. そのPRがどれぐらい必要なのかわかりずらい。
2. 各ロボットで同じような開発がされているのでDiscussonを希望したい。

1)は

• label は誰でも付けたり外したりできるものでしょうか？　wait for upstream, wait for fix, review request 等々。マージを検討する段階にあるのかないのか。可視化されるとよいです。
• review は誰でもできるでしょうか？そのコードを使っている人がいたらreview してくれるとPRの重要性が可視化されます。
• github action の restart は誰でも出来るでしょうか？数回CI走らせれば取る場合は積極的にretart してくれるとよいです。

[k-okada]

Milestone を活用するのはどうでしょう。
-> https://github.com/jsk-ros-pkg/jsk_robot/milestone/4

• Milesotne はEdit出来るかな？出来ればここに，このマイルストーンが達成されると何が出来て，どこで発表されるか書いてあるとMergeがはかどりそうです。

[knorth55]

@sktometometo
master-with-pull-requestはconflict確認用ってことかな？
conflictはどうしても起こるので、その場でちょくちょく直していくしかないですね。。。
knorth55/jsk_robot@fetch15は人力で北川が直しています。

はじめに：鶏と卵

個人的にはPRは自分が思う単位で出してもらえるとありがたいです。
たしかにfetch15はorigin/masterからかなりのcommit数があって、絶望感からPRを出さない、多くのcommitが入ったPRを出すには大変すぎる、というのは個人的な感想です。
なので、根本的にはorigin/masterとのcommit数のdiffを減らすことなんじゃないかなと思います。
で、それをやるためにはPRを出さないといけない、というお話しで、鶏か卵かという堂々巡りに入りましたね。
さてどうしましょう？

PRの分類

PRを分類してみると大きく下の4つくらいに上げられるのかなと思いました。

• 致命的なバグを修正するPR
• 新たな機能を追加するPR
• 現在の機能のリプレイスするPR
• /etcなどのロボット設定を変更するPR

致命的なバグは比較的commitも少ない傾向にあり、PRも出すのが簡単で、結構せっつけばすぐにマージしてもらえている印象があります。
現在の機能のリプレイスはcommitが多くなる可能性もありますが、新たなことができない限りはあまりマージされる見込みもないので、自己満足と言えるでしょう。
問題は新たな機能を追加するPRと/etcなどのロボット設定の変更のPRかなと思います。
前者はたとえばmove_baseの機能をいじる、controllerを変えるなど、後者はsupervisorのジョブを変えるなどですかね。

新たな機能を追加するPR

個人的に一つ解決策として考えていたのは#1149 のようにデモ単位でPRを出すことです。
#1149 には多くの依存があるのですが、デモが動いていることを示せばマージしてもらえるかな、と思っていたのですが、そういうわけではなかったようです。
（ちなみに #1149 は最新にアップデートされていないのでマージされるとおそらくconflictの原因になります）
となると、デモが動いて論文にならないとマージされないのかな？という仮説が立てられます。
この仮説に対して北川はまだ論文を書けていないので検証が行われていないのですが、塚本くんの卒論や大日方くんの卒論がマージされたのかな、とみていると、卒業論文ではダメ？なのかなというのが所感です。
以上から、新たな機能を追加するPRについては、マイルストーンを決めて論文を発表していくという岡田先生の提案があっていると個人的には思います。
（北川みたいに信頼を失うとどんどんマージされなくなるので、論文を発表してマージしてもらわないといけないですね）

/etcなどのロボットの設定を変更のPR

ロボットの設定を変更するPRが一番厄介かもしれません。
これは新たな機能を追加するという面がある反面で、現在の機能をリプレイスしているという面も大きくあるからです。
実際にFetchのMelodic移行に関する/etc以下の変更のPR #1208 のみがgo-to-kitchen #1149 の中でマージされずにいます。
ほかにも #1208 #1323 #1328 #1332 #1337 #1465 などがあげられます。
これも #1328 のケースをみていると、マージがされないことはない、という感じですが、これには以下の２つが懸念されていてマージされないのだと考えています。

• ロボットがその設定で安定的に動くのか
• すぐに設定が変わるのではないか

1つめの安定的に動くかについては、新機能の追加と同じでマイルストーンを決めていく同様の手法が有効に思えます。
問題は2つめの「すぐに設定が変わるのではないか」です。
これについてはLinuxの諸々が変わっていくこと (i.e. init.d -> service -> initctl -> systemdやnetworking -> netplan) への懸念と、最適なものはこれだという意見の相違という２つが主に挙げられます。
顕著に現れているのが systemd v.s. supervisordに関する問題です。
現にsupervisorに関するPRは、岡田先生はどうせsystemdへ移行するからそっちに移行するのをずっと待たれているように感じますが、Fetchユーザからはsupervisorで動いているからまぁいっか、という感じで平行線を辿っているように思えます。(c.f. #1323 #1465)
systemd vs supervisordに関しては、各ロボットに対してroslaunchが１つであればsystemd, supervisordでもどれでもいいのではないか、というのが私の感想で、その議論については #1465 で行われていました。
個人的にはsystemdでもsupervisordでもどちらでも同じ挙動なのでどっちでもよいと思っているのですが、github上での議論が発散してどこにも結論がない、というのが現状だと思います。
これについてはオフラインで議論するのが手っ取り早いと思いますが、じゃあ誰がやるのか？という話になると現状動いているもののリプレイスメントなので、誰かに勧めることもできず、だれかがやるのを皆が待っている状態になっているかと思います。
最初に海に飛び込むペンギンと同じですね。えいやとやった人が損をするという構図です。
なので北川がやってしまってよいと個人的には思っているのですが、それをやるから後輩がうんぬんかんぬんと言われるので、うーん、どうしようかなという気持ちでもあります。

また#1208 の例にあるように、Merge afterと書いてあるのに先にマージされてしまい、放置されてしまう傾向もあるようです。
先にマージされてしまったケースは残り滓しかPRに含まれておらず、現状どうすることもない、マージされるのは望み薄だなぁという感想です。
なにかのときにうまくマージされたら嬉しいな、くらいが個人的な感想です。

さいごに：jsk_robotはリリースというマイルストーンを失った

jsk_robotはindigoまではdebをリリースされていました。
しかしbaxterやfetchなどリリースをしないパッケージが存在するため、kinetic melodicではリリースをされなくなりました。
このリリースの機会というのが一つのマイルストーンだったはずですが、この機会をjsk_robotは失ってしまいました。
jsk_robotをソースで入れるというのが当たり前になった今、branchをわけてそのbranchをビルドすればいいわけで、そうなるとorigin/masterが活発にマージされないレポジトリがbranch基準で開発されていくのは開発者としては当たり前になってしまって、origin/masterからの乖離が多くなっていくのかなとおもいます。
リリースでもいいですが何かしらの時期的なマイルストーンを設定することは良いと思います。