atcodertools.client を online-judge-tools で置き換える

[kmyk]

以前から話をしているものです。

次の2点が嬉しさです:

1. module をひとつ丸ごと消せるので保守性の向上に繋がる
2. AtCoder 以外のコンテストサイトへの対応が容易になる。特に yukicoder のコード生成が可能になる

注意点としては:

1. コードが外部のライブラリに移るので修正速度の低下がある
2. 依存先のバージョン管理の面倒が発生しうる

[kmyk]

WIP で止まっているのは online-judge-tools/oj#380 の問題を解決しないと完了させられないためです。

[codecov-io]

Codecov Report

Merging #134 into master will decrease coverage by 1.75%.
The diff coverage is 82.66%.

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
- Coverage   89.85%   88.09%   -1.76%     
==========================================
  Files          53       50       -3     
  Lines        2582     2419     -163     
==========================================
- Hits         2320     2131     -189     
- Misses        262      288      +26

Impacted Files	Coverage Δ
...codertools/constprediction/constants_prediction.py	93.84% <100%> (+1.53%)	⬆️
...ols/constprediction/models/problem_constant_set.py	100% <100%> (ø)	⬆️
atcodertools/tools/envgen.py	81.48% <100%> (ø)	⬆️
atcodertools/tools/models/metadata.py	100% <100%> (ø)	⬆️
atcodertools/client/atcoder.py	66.66% <75%> (-23.71%)	⬇️
atcodertools/client/models/problem_content.py	85% <76%> (-10.24%)	⬇️
atcodertools/tools/codegen.py	72.15% <83.33%> (-5.17%)	⬇️
atcodertools/fmtprediction/models/format.py	91.93% <0%> (-4.84%)	⬇️
...tcodertools/fmtprediction/predict_simple_format.py	95.31% <0%> (-3.13%)	⬇️
... and 4 more

---

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 1126750...09ab5c7. Read the comment docs.

[kmyk]

テストが gzip された HTML を読んで実行しており、現状の online-judge-tools にはそのような機能はないのでこれを処理できませんでした。
具体的には以下のふたつです。

atcoder-tools/tests/test_constpred.py

Lines 36 to 37 in 778c36c

	@unittest.expectedFailure # TODO
	def test_predict_constants(self):

atcoder-tools/tests/test_constpred.py

Lines 69 to 70 in 778c36c

	@unittest.expectedFailure # TODO
	def test_case_only_with_no_str(self):

[kyuridenamida]

@kmyk
あー!確かに定数抽出は生htmlをそのまま使っていますね。本質的には問題文パートとかサンプル説明とかそのへんが取れればhtmlじゃなくてもいいんですけれども、生htmlダウンロード機能(つまりサンプル抽出とかのために使ったhtmlをそのまま生で返すAPI)ってonline-judge-tools的には対応する予定はないですか?

[kmyk]

@kyuridenamida 生HTMLダウンロード機能は何も検討していませんでした。とりあえずは requests.get(problem.get_url()).content を使う形で誤魔化させてほしいです。

あってもよさそうですが、何も考えずにライブラリの公開APIとして足すには仕様についていくつか検討しないといけない点があり、その判断が難しいためです。具体的には次の2点です:

• 404 や 503 の場合の処理はどうするか。problem not found みたいなページを 200 で返してくるサイトはどう扱うか。
• 内容はキャッシュするのか。したい気はするが、さすがに get_raw_html のような名前で雑なキャッシュをするとよくなさそう。例えば Codeforces のコンテスト中の問題ページの右下の「現在の点数」の部分などは頻繁に変化する

[kmyk]

とりあえず、入力部分やサンプル説明を (毎回解析して取得するのでなくて) 直接保存しておいて使う方向を試そうとしています。

[kyuridenamida]

返事おそくなりましたが納得です。ありがとうございます。最近僕側の反応速度が律速になっていて申し訳無さを感じています

[kmyk]

@kyuridenamida 生HTMLを操作するAPIをonline-judge-toolsに足して、とりあえず実装はできました。
しかし tests/resources/common/problem_htmls.tar.gz や tests/resources/common/test_data.tar.gz に入っているデータが旧AtCoderのものなためにテストが通らないのです。ここの更新は雑に (内容の手動での確認はせずに) やっていいですか？

[kmyk]

個人的な意見としては、このような形のすべてを舐めるようなテストは良くないと思っています。
理由としては:

• 更新をきちんとしないと最新のものへのテストが丸ごと抜けることになる。しかも一見して抜けているようには見えない
• テストケースの中に間違ったものが入る。修正すると zip ファイルを丸ごと更新する必要が発生し .git が膨らむ (ちなみに git は何もしなくてもファイルを圧縮して保存してくれます)
• どのケースで失敗したのかが分かりにくい
• テストのドキュメントとしての機能が弱くなる傾向がある

テストとして使う問題をいくつか選んでそれだけを扱う形に書き直すのでもよいですが、どう思いますか？

[kyuridenamida]

@kmyk さん

@kyuridenamida 生HTMLを操作するAPIをonline-judge-toolsに足して、とりあえず実装はできました。
しかし tests/resources/common/problem_htmls.tar.gz や tests/resources/common/test_data.tar.gz に入っているデータが旧AtCoderのものなためにテストが通らないのです。ここの更新は雑に (内容の手動での確認はせずに) やっていいですか？

ありがとうございます!!!はい、大丈夫です。こういう一番バグりそうな瞬間に既存のテストが役に立たないのはかなり悔しさがありますが、各コンテストに対する成功率が変化しているかどうかについては僕もマージ前に確認します!

個人的な意見としては、このような形のすべてを舐めるようなテストは良くないと思っています。

この網羅テストのおかげでコア機能に関するバグを防いでいる実感があるので、半分くらい反対です。このテストが無いとして入力解析アルゴリズム(例えば正規化方法とか)を弄ったときにどの問題に対する解析が壊れるか想定・網羅してユニットテストを厳選して書いておくのは人的コストに見合わないので、単にdiffを検知するツールとして持っておきたいという気持ちがあります。

・・・が、半分くらい同意です。仰る通りあのテストにドキュメントとしての機能はほぼありません。かなり不便です。端なるdiffを検知するテストのためのzipを濫用していくつもの重要なテストを汚染しているのは雑なので、あのテストの代わりにドキュメント性の高い開発時に役に立ちそうないくつかのきちんとした小さいユニットテストを書くということには賛成です。

他の指摘に関するコメントです。

更新をきちんとしないと最新のものへのテストが丸ごと抜けることになる。しかも一見して抜けているようには見えない

実はあのテストを最新に保つことに必要性は強く感じておらず、主に焦点を当てていたのは、単にデータの量の大小です。そういう理由で過去問だけでも十分な量のサンプルだと思っていて、更新しなくても正直そこまで支障が出ないと考えています。入力形式によほど大きな変更が起きたときにこの楽観視は終焉を遂げるのですが、さすがにissueが飛んでくるかなと思っています。

テストケースの中に間違ったものが入る。修正すると zip ファイルを丸ごと更新する必要が発生し .git が膨らむ (ちなみに git は何もしなくてもファイルを圧縮して保存してくれます)

間違ったものが入っているのは単にテストケースが多い & 僕が1つ1つ吟味するのが面倒くさかったという事実の組み合せに起因するものです。申し訳ない。そこで、"想定解"の定義を「現状と同じ動作である」という風に誤魔化してdiffツールとして使っています。zipの更新に関しては、今回は特にhtmlではなく中途半端な形式で圧縮していることが主な理由だと思っていて、htmlで圧縮しておけば良かったと思っています...。とはいえそもそもzipを使うことによって.gitが膨らむのは知らず、完全にgit初心者ムーブでした。gitで圧縮ファイルはやめます...(前教えてもらったのに放置しててすみません。)

どのケースで失敗したのかが分かりにくい

これはテストの書き方次第でどうとでもなりそうです。

:

上記を踏まえた上で以下の2ステップの作業の提案です。

1. @kmyk さんは今テスト落ちてるやつを通すように好きな方法で修正する、例えばzip消してtest_overallとかを消して、いくつかの本質的なユニットテストだけを追加するとかです。
2. 中途半端に解析した結果を保存しているのがメンテ性を著しく損なっているので、単にhtmlを固めたものを適当なストレージからダウンロードできるようにしておいてそれを使ってフルテストする。
   (@kyuridenamida が別issueでいつかやる)

• 勝手にzip側が変わってテスト落ちると悲しいのでzipファイルはimmutableにします。あと、コンテストが増えても必要性がない限りそんなに更新はしません。
• zipはこのテストだけに使うものとします。他のところで乱用するとドキュメント性が落ちるので。

どうでしょうか。

いつもかなり建設的な議論を持ちかけてくれて、助かっています。

[kmyk]

@kyuridenamida

提案されたものはよさそうです。
test_overall はとりあえず「ドキュメントとしても働くような検証された小さなテスト」だけに絞って、「diff を取る意味での網羅テスト」としてフルテストを別に設けるという形であれば、役割も明確ですし要求もすべて満たされます。
そのような形でユニットテストを修正しようと思います。

ただし、フルテストを CI の範囲に含めるかどうかについては確認しておきたいです。
diff の範囲に online-judge-tools の挙動を含めてしまうと「atcoder-tools では何も悪いことをしていないのに急に CI が落ちるようになる」といった事態が発生する可能性があります。(実際に online-judge-tools の例では、 POJ の鯖が落ちたので数日の間ずっと CI が通らなかったということがありました。)
両方を同時に使うユーザが想定できるので、 atcoder-tools の側で強いバージョン指定をするという解決策も難しいです。

コメントについても特に異論ありません。
「欠けがあるのにすべてテストしているように見えてしまう」系の状態はよくないということは再度主張しておきたいですが、今回のものは大丈夫そうだというのはたしかにその通りです。
失敗したケースの特定の問題はよく考えれば当然できるはずで、実際にサブテストという形で存在していました。これは自分で気づくべきでした。

---

ところで、少なくともサンプルケースの取得のフルテストについては online-judge-tools の側でも行おうと思います。参加者の提出のダウンロード機能とコンテナ技術をあわせれば精度ほぼ100%が達成できるはずということに気付きました。

いつもかなり建設的な議論を持ちかけてくれて、助かっています。

これですが、こちらこそありがとうございます。議論に丁寧に付き合ってもらえるので新しい発見が多くて勉強になります。

[kyuridenamida]

すみません、返事が必要系のコメントだと認識しておらず、放置していました。

フルテストをCIに含めるかどうかですが、少なくともatcoder側の通信部分くらいはonlinejudgetoolsの挙動の影響を受けても良いです。それが壊れてるとそもそもユーザーが使えないですし、どちらにしろ検出されるべきシチュエーションではあると思います。

ところで、少なくともサンプルケースの取得のフルテストについては online-judge-tools の側でも行おうと思います。参加者の提出のダウンロード機能とコンテナ技術をあわせれば精度ほぼ100%が達成できるはずということに気付きました。

これすごい、堅牢なライブラリを作る達人

[kmyk]

@kyuridenamida テストの修正ができました (2か月も待たせてしまってごめんなさい)
online-judge-tools 側のログもまざって出力されてしまう問題も発生しているので、次はこれを解決します。それが終われば [WIP] を外せるはずです。

[kyuridenamida]

これかなりさっさとCR終えてマージすべきだったな、後回しにした結果コンフリクトが発生するやつですね...頑張って終わらせます

[kyuridenamida]

こんだけ待たせてしまったのに実はunused import以外指摘するところがなくて若干申し訳ないですが、一応CR終わりました。ほぼマイナー指摘なので、このままMRしてもいいくらいです。かなり素晴らしい修正なのに放置し続けてしまったことを悔いています。ありがとうございます&すみません。

[kmyk]

conflict の解消もしたのですが、マージはひとまず待ってほしいです。
online-judge-tools 側の設計ミスを除去するための大きめの更新が予定されているためです。

[kmyk]

@kyuridenamida conflict の解消と online-judge-tools の依存するバージョンの安定版への更新をしまし、マージできる状態になりました (おまたせしました)

[kyuridenamida]

@kmyk
完全にコメントを見逃していていました。あまりにも申し訳ないのでこちらでマージコンフリクト解消してマージしてもいいですか?本当にｽﾐﾏｾﾝ...

[kmyk]

@kyuridenamida マージはしてもらって大丈夫です。そちらでコンフリクト解消してもらえるのならとても助かります。

---

ところで、マージするにせよしないにせよ、このプルリクをマージすべきかどうかは現状を踏まえて検討しなおすべきだと思っています。reject すべきだと判断したならきちんと reject してください。

当初想定していた利点は次のふたつでした。

1. コードの共通化により atcoder-tools のメンテコストの削減になる
2. atcoder-tools を AtCoder 以外のサイト (たとえば Codeforces とか) にも対応させるのに役立つ

しかし、どちらの利点も薄くなっている気がしています。
(1.) のメンテコストの問題は無視できるようになっているはずです。atcoder-tools へのコントリビューターがかなり増えているためです。レビューの問題はありますが、いまならレビューを任せられる人も探せばすぐ見つかりそうです。
(2.) はいまも価値があると思っている (たとえば LeetCode 対応は需要があってかつコード生成機能との連携が重要となる) のですが、私が忙しいためにそのような続きのプルリクを書けそうにありません。中途半端な状態で放置することになりそうで、これはあまりよくありません。

[kyuridenamida]

coder-tools を AtCoder 以外のサイト (たとえば Codeforces とか) にも対応させるのに役立つ

僕は実はこれに希望を感じている人です。
確かにこれ以外でメリットがあるか?というとあんまり思いつかないですね。いやでもこれはこれで大きいメリットだと思うのでせっかくなら...という気持ちがあります。ちょっと考えさせてください。どの道将来的に何か役に立ちそうかつコード変更として完成しているならリジェクトする気はあんまり無かったです。

[kmyk]

これとりあえず閉じておきますね。conflict 解消たいへんでそう簡単にはマージできないだろうし、大きいプルリクが Open のまま残ってると他の人がプルリク出しにくそうなためです。