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.

Sign up for GitHub

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

Jump to bottom

style: 💄 add pylint, black and other tools #52

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

style: 💄 add pylint, black and other tools #52

wants to merge 3 commits into from
1,516 −27

Conversation

david30907d
Copy link
Contributor

@david30907d david30907d commented Aug 7, 2020 •
edited
Loading

提議：我平常喜歡把這些 linter, formatter 綁在 git hook 上，每次 commit 自動檢查，不確定會不會跟大家的習慣衝突

  • 套件掃到一些 tensorflow 的資安問題，想問如果升版號的話，會有什麼影響（客戶之類的？）
  • 然後我在 local 用 npm install 會噴很多 error，想問大家現在還能正常安裝嗎？
  1. Use pylint to check camera.py
  2. Turn logging.info(...format()) into logging.info( % ) according to logging-format-interpolation
  3. Use safety to check our installed dependencies for known security vulnerabilities.
╒══════════════════════════════════════════════════════════════════════════════╕
│                                                                              │
│                               /$$$$$$            /$$                         │
│                              /$$__  $$          | $$                         │
│           /$$$$$$$  /$$$$$$ | $$  \__//$$$$$$  /$$$$$$   /$$   /$$           │
│          /$$_____/ |____  $$| $$$$   /$$__  $$|_  $$_/  | $$  | $$           │
│         |  $$$$$$   /$$$$$$$| $$_/  | $$$$$$$$  | $$    | $$  | $$           │
│          \____  $$ /$$__  $$| $$    | $$_____/  | $$ /$$| $$  | $$           │
│          /$$$$$$$/|  $$$$$$$| $$    |  $$$$$$$  |  $$$$/|  $$$$$$$           │
│         |_______/  \_______/|__/     \_______/   \___/   \____  $$           │
│                                                          /$$  | $$           │
│                                                         |  $$$$$$/           │
│  by pyup.io                                              \______/            │
│                                                                              │
╞══════════════════════════════════════════════════════════════════════════════╡
│ REPORT                                                                       │
│ checked 72 packages, using default DB                                        │
╞════════════════════════════╤═══════════╤══════════════════════════╤══════════╡
│ package                    │ installed │ affected                 │ ID       │
╞════════════════════════════╧═══════════╧══════════════════════════╧══════════╡
│ tensorflow                 │ 1.14.0    │ <1.15.0                  │ 37524    │
╞══════════════════════════════════════════════════════════════════════════════╡
│ The original changelog reads: "Tensorflow 2.0 fixes a potential security     │
│ vulnerability where decoding variant tensors from proto could result in heap │
│ out of bounds memory access." However, it was later confirmed that the fix   │
│ was already included in 1.15 and later. See:                                 │
│ <https://github.com/tensorflow/tensorflow/issues/37701>.                     │
╞══════════════════════════════════════════════════════════════════════════════╡
│ tensorflow                 │ 1.14.0    │ <1.15.3                  │ 38462    │
╞══════════════════════════════════════════════════════════════════════════════╡
│ tensorflow 1.15.3 * Updates `sqlite3` to `3.31.01` to handle CVE-2019-19880, │
│ CVE-2019-19244 and CVE-2019-19645 * Updates `curl` to `7.69.1` to handle     │
│ CVE-2019-15601 * Updates `libjpeg-turbo` to `2.0.4` to handle                │
│ CVE-2018-19664, CVE-2018-20330 and CVE-2019-13960 * Updates Apache Spark to  │
│ `2.4.5` to handle CVE-2019-10099, CVE-2018-17190 and CVE-2018-11770          │
╞══════════════════════════════════════════════════════════════════════════════╡
│ tensorflow                 │ 1.14.0    │ >=1.0,<1.15.2            │ 38038    │
╞══════════════════════════════════════════════════════════════════════════════╡
│ Tensorflow 1.15.2 and 2.0.1 update `sqlite3` to `3.30.01` to handle          │
│ CVE-2019-19646, CVE-2019-19645 and CVE-2019-16168.                           │
╞══════════════════════════════════════════════════════════════════════════════╡
│ tensorflow                 │ 1.14.0    │ >=1.0,<1.15.2            │ 37776    │
╞══════════════════════════════════════════════════════════════════════════════╡
│ In TensorFlow before 1.15.2 and 2.0.1, converting a string (from Python) to  │
│ a tf.float16 value results in a segmentation fault in eager mode as the      │
│ format checks for this use case are only in the graph mode. This issue can   │
│ lead to denial of service in inference/training where a malicious attacker   │
│ can send a data point which contains a string instead of a tf.float16 value. │
│ Similar effects can be obtained by manipulating saved models and checkpoints │
│ whereby replacing a scalar tf.float16 value with a scalar string will        │
│ trigger this issue due to automatic conversions. This can be easily          │
│ reproduced by tf.constant("hello", tf.float16), if eager execution is        │
│ enabled. This issue is patched in TensorFlow 1.15.1 and 2.0.1 with this      │
│ vulnerability patched. TensorFlow 2.1.0 was released after we fixed the      │
│ issue, thus it is not affected. Users are encouraged to switch to TensorFlow │
│ 1.15.1, 2.0.1 or 2.1.0. See: CVE-2020-5215.                                  │
╞══════════════════════════════════════════════════════════════════════════════╡
│ tensorflow                 │ 1.14.0    │ >=1.0,<1.15.2            │ 38039    │
╞══════════════════════════════════════════════════════════════════════════════╡
│ Tensorflow 1.15.2 and 2.0.1 update `curl` to `7.66.0` to handle              │
│ CVE-2019-5482 and CVE-2019-5481.                                             │
╞══════════════════════════════════════════════════════════════════════════════╡
│ tensorflow                 │ 1.14.0    │ >=1.0.0,<1.15.2          │ 38549    │
╞══════════════════════════════════════════════════════════════════════════════╡
│ Tensorflow 1.15.2 and 2.0.1 fix a security vulnerability to address          │
│ CVE-2020-5215 where converting a Python string to a `tf.float16` value       │
│ produces a segmentation fault.                                               │
│                                                                              │
│ Both also update `curl` to `7.66.0` to address CVE-2019-5482 and             │
│ CVE-2019-5481.                                                               │
│                                                                              │
│ Both also update `sqlite3` to `3.30.01` to address CVE-2019-19645,           │
│ CVE-2019-19646, CVE-2019-16168.                                              │
╘══════════════════════════════════════════════════════════════════════════════╛

@david30907d david30907d changed the title perf: ⚡️ fff style: 💄 add pylint, black and other tools Aug 7, 2020
@bafu
Copy link
Member

bafu commented Aug 16, 2020

@david30907d Thanks for the PR! I will provide my comments aside from the source codes.

bafu
bafu reviewed Aug 16, 2020
View reviewed changes
berrynet/client/camera.py Outdated
#
# You should have received a copy of the GNU General Public License
# along with BerryNet. If not, see <http://www.gnu.org/licenses/>.
"""
Copy link
Member

Choose a reason for hiding this comment

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

We will keep license description in the comment header instead of module docstring.

Module docstring is for providing the high-level description of the module.

Example: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/ops/accumulate_n_benchmark.py

bafu
bafu reviewed Aug 16, 2020
View reviewed changes
berrynet/client/camera.py Outdated
'--mode',
default='stream',
help='Camera creates frame(s) from stream or file. (default: stream)'
"""
Copy link
Member

Choose a reason for hiding this comment

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

Because the function name is self-explanatory, I suggest to remove the docstring.

If the goal is for generating the development document in the future, we can make a plan to provide informative docstrings in the functions and modules.

bafu
bafu reviewed Aug 16, 2020
View reviewed changes
berrynet/client/camera.py Outdated


def main():
"""
Copy link
Member

Choose a reason for hiding this comment

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

The same as the "args" docstring.

bafu
bafu reviewed Aug 16, 2020
View reviewed changes
berrynet/client/camera.py Outdated
)
ap.add_argument(
'--stream-src',
Copy link
Member

Choose a reason for hiding this comment

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

As described in PEP8:

In Python, single-quoted strings and double-quoted strings are the same. This PEP does not make a recommendation for this.

We choose single quote because most of the Python official docs use single quote.

Example: https://docs.python.org/3/tutorial/controlflow.html#for-statements

However, we are open to adopt a new rule if there is a good reason behind. It will be great if you can share more about your opinions with me. 😃

https://stackoverflow.com/questions/56011/single-quotes-vs-double-quotes-in-python

@bafu
Copy link
Member

bafu commented Aug 16, 2020 •
edited
Loading

More comments for the questions in the PR:

  1. 提議：我平常喜歡把這些 linter, formatter 綁在 git hook 上，每次 commit 自動檢查，不確定會不會跟大家的習慣衝突

如果 linter 的修改建議是 optional 而非 necessary，那麼加上 linter 沒有問題

BerryNet dev is goal driven with startup mindset: 有效率地解決問題是 1st priority。有 linter 絕對很棒，但希望它是提供建議者而非強制性的 blocker

  1. 套件掃到一些 tensorflow 的資安問題，想問如果升版號的話，會有什麼影響（客戶之類的？）

TensorFlow 升版最常見的問題就是 API compatibility issues (API breaks)，建議升版前先確認客戶或產品所使用的 BerryNet engine / service 不會受到影響

RaspberryPi 上的 TensorFlow，是使用朋友 PINTO0309 的 Tensorflow-bin，通常PINTO0309 會跟上最新版，若要更新 RPi TF，建議先看看 PINTO0309 是否已提供支援

  1. 然後我在 local 用 npm install 會噴很多 error，想問大家現在還能正常安裝嗎？

許多 BerryNet JS clients 還是 BerryNet v1 的架構 (目前是 v2)，因此無法安裝是符合預期的

如果你希望能用 JS clients，建議可以先開 GitHub issue 留個紀錄，dev priority 就會提高

  1. Use pylint to check camera.py

The same as the item 1.

  1. Turn logging.info(...format()) into logging.info( % ) according to logging-format-interpolation

建議當確認效能受到影響，再修正 logging level >= INFO 的 statements.

不希望強制執行此 rule 的原因：

  • DEBUG level 在 product 中不會開啟
  • Formatter 在 debugging 時，可以有彈性、具可讀性地調整輸出參數
  1. Use safety to check our installed dependencies for known security vulnerabilities.

聽起來很棒 👍

Thanks for all the great inputs in this PR!

@david30907d
Copy link
Contributor Author

  1. 也對，不應該強制，抱歉哈哈。我檢查完確認現在沒有強制的設定，除非執行 npm run commit 才會走 linter, formatter 的流程
  2. 好麻煩我選擇死亡XD
  3. 好的我來開
  4. 了解～
  5. 喔喔，的確 format() 比較有可讀性，不過即使 logging level 在 prod 設定 >= INFO，logging.debug(format) 還是會真的把字串 render 完然後看一下 logging 的 level 才決定不顯示，會浪費一點效能，根據 logging 的文件:

Formatting of message arguments is deferred until it cannot be avoided. However, computing the arguments passed to the logging method can also be expensive, and you may want to avoid doing it if the logger will just throw away your event.
；反之 logging.info( % ) 有 provide lazy interpolation 的特性。想說在 edge device 上能省就省，但我跟 edge device 不熟所以只是建議XD

@david30907d
style: 💄 add pylint, black and other tools
9be844b 
These tools would help us maintain our code quality
@david30907d
test: 💍 generate report of test coverage
79d4b04
@david30907d
style: 💄 add flag for black and remove comment
a8825f4 
skip-string-normalization can ignore quote normalization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bafu bafu Awaiting requested review from bafu

Assignees

@david30907d david30907d

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@david30907d @bafu