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

Feature/dic with zipimport #54

Merged
merged 6 commits into from Sep 17, 2018

Conversation

roy-ht
Copy link
Contributor

@roy-ht roy-ht commented Sep 6, 2018

問題点

  • 現状のFSTバイナリの読み込み方がパッケージ下のファイル読み込みになっている
  • zipimportしたい案件で失敗する(パッケージをzipで固めてimportする方法)

例:

import sys
sys.path.append('janome.zip')
from janome.tokenizer import Tokenizer
t = Tokenizer()  # ここで死ぬ

解決案

  • 標準のpkgutil.get_data()を用いて、パッケージからバイナリを読み出す
  • パフォーマンスへの影響がわからないため、失敗したときのみpkgutilを使う方式にフォールバックするようにする

宜しくおねがいします🙏

@coveralls
Copy link

coveralls commented Sep 6, 2018

Coverage Status

Coverage increased (+0.05%) to 87.448% when pulling 708f5bf on roy-freee:feature/dic_with_zipimport into eb211f0 on mocobeta:master.

@mocobeta
Copy link
Owner

mocobeta commented Sep 7, 2018

PRありがとうございます。

zipimport は使ったことがなく,手元でまだ試していないのですが,zip アーカイブの作成手順と簡単なサンプルスクリプトをいただけますか?
examples に例を追加していただけるとより良さそうです。

パフォーマンスへの影響がわからないため、失敗したときのみpkgutilを使う方式にフォールバックするようにする

辞書の読み込みは Tokenizer() の初期化時なので,パフォーマンス全般への影響はさほど考慮しなくてもいいと思います(つまり load_all_fstdata_from_package() に統一で良さそう)。

@roy-ht
Copy link
Contributor Author

roy-ht commented Sep 12, 2018

コメントありがとうございます。

examplesに例を追加しました。手元で実行して確認済みです。

パフォーマンス

言葉足らずで申し訳ないです。
初期化コストはいいのですが、gzipと違い、ストリーム読み込みではなく一旦bytesで全圧縮バイト列を読み込んでからdecompressで展開するので、ピーク消費メモリを気ににする人には辛いかなと思いこのような実装にしてみました。

因みに主な用途はAWS系(例えばLambdaとか。)で使えて嬉しい、という感じです。

@mocobeta mocobeta merged commit 708f5bf into mocobeta:master Sep 17, 2018
@mocobeta
Copy link
Owner

@roy-freee スクリプト追加ありがとうございます。いくつか動作確認をしてマージしました。

  • usage_with_zipimport.py の説明に,クレジットと制限事項を追記しました。See: 769ad2c
  • しばらくリリース予定がないので少し先になるかもしれないですが,次のリリース(0.3.7) から,(sdist と併せて) universal wheel も PyPI にアップロードしたいと思います。

Thanks so much for your contribution!

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

3 participants