Skip to content

Feature/issue100 - #1

Merged
satoshi-kondo merged 9 commits into
mainfrom
feature/issue100
Nov 10, 2025
Merged

Feature/issue100#1
satoshi-kondo merged 9 commits into
mainfrom
feature/issue100

Conversation

@satoshi-kondo

Copy link
Copy Markdown
Contributor

Issue リンク / Link to Issue

https://github.com/airoa-org/hsr_devops/issues/100

やったこと / What I did

  • データ収集の start/complete 時に common と device の facet を追加
  • 上記に合わせてサンプルコードを更新

やってないこと / What I did not

  • 上記の facet 以外を追加すること

チェック観点 / Check Points

  • "common", "device" と facet を分けているがこれで問題ないか確認してほしい
  • フィールド名を Marquez に合わせて、lowerCamelCase を利用しているがこれで問題ないか確認してほしい

補足 / Supplement

  • 上記のチェック観点の仕様を固めつつ、今後の Facet 追加に反映させていければと思います。

@satoshi-kondo

Copy link
Copy Markdown
Contributor Author

サンプルコード実行で Marquez に格納されるイベント

START
image

COMPLETE
image

def __init__(
self,
namespace: str,
common_facet: CommonRunFacet,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

common, device の Facet については init 時に設定し、start/complete イベント作成時に利用する形で実装。

@yuya-haruna yuya-haruna left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

コメントしています!

@@ -0,0 +1,23 @@
"""OpenLineage facets for teleoperation tracking."""

import attr

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

pyproject.tomlにattrsを追加いただいた方が良さそうです。
(たまたま openlineage-python が依存しているから入っている?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

image

はい。openlineage-python で利用しているので特に問題なく使えている形になります。
こういったケース、何か問題があるまで、自分で pyprojext.toml の方にライブラリを追加していなかったのですが、こちら、追加しておいた方が良さそうですかね? こういった観点があるので、追加しているとかあれば教えていただきたく。 🙏

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

何か問題があるまで、自分で pyprojext.toml の方にライブラリを追加していなかったのですが、こちら、追加しておいた方が良さそうですかね?

使うなら明示的にpyproject.tomlに書いてあった方が良さそうな気はしつつ、実害はないので入れなくても良いのかなという温度感です。
(使っているライブラリから依存が外れる心配がなさそうなライブラリなら)

ちなみにcodexの主張は以下でした

こちらのパッケージ自身が `import attr` を直接呼んでいる以上、「自パッケージが必要とする依存関係」として `attrs` を明示するのが Python packaging のベストプラクティスです。

現在は `openlineage-python` がたまたま `attrs` に依存しているため環境によっては問題が顕在化しないだけで、将来その依存が削除・変更された瞬間に本パッケージが壊れるリスクが残ります。直接 import しているライブラリは、自身の `pyproject.toml` へ一次依存として記載するようご検討ください。

... repositoryBranch="main"
... )
>>> device_facet = DeviceRunFacet(hostname="operator-pc-001")
>>> session = TeleopSession(

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

README.mdexamples.mdなどの記述の更新もお願いします
(二重管理になるのでmdかコードかどちらかのみにできると良いのかなという気もしました)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

0864f6c
上記コミットで以下、2点の対応を行いました。

  • README.md を最新にする
  • examples.md を削除する

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

すみません、development.mdにも古い記述が残っていそうなので修正お願いします🙏

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

すみません、
architecture.mdにも残ってました🙇‍♂️

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

対応しました。

Comment thread examples/teleop_session.py
job_name: str = "robot-data-collection",
marquez_url: str | None = None,
run_id: str | None = None,
facet_prefix: str = "",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

required ではなく、optional として空文字を設定しています。
prefix を使わないケースとかも出てくる可能性がありそうだなと思いまして。

@yuya-haruna yuya-haruna left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

まだ未修正のドキュメントが残っていたので、そこ修正いただいたらLGTMです。

Comment thread docs/development.md Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ここも修正お願いします🙏

@satoshi-kondo
satoshi-kondo merged commit d29950a into main Nov 10, 2025
1 check passed
@satoshi-kondo
satoshi-kondo deleted the feature/issue100 branch December 25, 2025 07:37
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.

2 participants