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

fix: fix the error of pylint in demo directory #1900

Merged
merged 7 commits into from
Jun 8, 2022
Merged

fix: fix the error of pylint in demo directory #1900

merged 7 commits into from
Jun 8, 2022

Conversation

mangoGoForward
Copy link
Contributor

Signed-off-by: mango [email protected]

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Bug fix

  • What is the current behavior? (You can also link to an open issue here)
    Part resolved python: pylint check found many error #1883

  • What is the new behavior (if this is a feature change)?
    None

@github-actions github-actions bot added the docker openmldb compile image or demo image label May 29, 2022
@mangoGoForward mangoGoForward changed the title [WIP]fix: fix the error of pylint in demo directory fix: fix the error of pylint in demo directory May 29, 2022
@codecov
Copy link

codecov bot commented May 29, 2022

Codecov Report

Merging #1900 (3d0f936) into main (1ffa2e4) will decrease coverage by 0.04%.
The diff coverage is n/a.

❗ Current head 3d0f936 differs from pull request most recent head 3d5f595. Consider uploading reports for the commit 3d5f595 to get more accurate results

@@             Coverage Diff              @@
##               main    #1900      +/-   ##
============================================
- Coverage     75.73%   75.69%   -0.05%     
  Complexity      347      347              
============================================
  Files           613      613              
  Lines        117341   117059     -282     
  Branches       1009     1024      +15     
============================================
- Hits          88867    88606     -261     
+ Misses        28265    28244      -21     
  Partials        209      209              
Impacted Files Coverage Δ
...aradigm/openmldb/batch/window/WindowComputer.scala 67.85% <0.00%> (-13.34%) ⬇️
src/sdk/sql_request_row.cc 53.22% <0.00%> (-3.23%) ⬇️
...ridse/src/passes/physical/long_window_optimized.cc 76.39% <0.00%> (-2.49%) ⬇️
src/zk/dist_lock.cc 81.81% <0.00%> (-1.52%) ⬇️
src/storage/aggregator.cc 74.57% <0.00%> (-0.65%) ⬇️
hybridse/src/codec/fe_row_codec.cc 78.49% <0.00%> (-0.48%) ⬇️
src/client/tablet_client.cc 55.11% <0.00%> (-0.44%) ⬇️
src/sdk/sql_cluster_router.cc 54.84% <0.00%> (-0.38%) ⬇️
src/storage/mem_table.cc 89.13% <0.00%> (-0.28%) ⬇️
src/codec/codec.cc 69.82% <0.00%> (-0.26%) ⬇️
... and 18 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 6ecf686...3d5f595. Read the comment docs.

@github-actions
Copy link
Contributor

SDK Test Report

  75 files    75 suites   8m 10s ⏱️
163 tests 161 ✔️ 2 💤 0
204 runs  202 ✔️ 2 💤 0

Results for commit 3d0f936.

@github-actions
Copy link
Contributor

Linux Test Report

       57 files       193 suites   1h 0m 58s ⏱️
  8 673 tests   8 669 ✔️ 4 💤 0
12 744 runs  12 740 ✔️ 4 💤 0

Results for commit 3d0f936.

pylintrc Outdated Show resolved Hide resolved
@aceforeverd
Copy link
Collaborator

@vagetablechicken have a refactor in #1902, would u pls rework after that ? Currently you can just ignore predict_server.py and train_and_serve.py

@mangoGoForward
Copy link
Contributor Author

OK.

@aceforeverd
Copy link
Collaborator

OK.

I think the two file should be fine after merge. If you are interested in styling from pylint, feel free to comment in #1902

@aceforeverd aceforeverd added code-standards code style/convention things python Pull requests that update Python code labels May 31, 2022
Signed-off-by: mango <[email protected]>

# Conflicts:
#	demo/talkingdata-adtracking-fraud-detection/predict_server.py
#	demo/talkingdata-adtracking-fraud-detection/train_and_serve.py
@mangoGoForward
Copy link
Contributor Author

@vagetablechicken have a refactor in #1902, would u pls rework after that ? Currently you can just ignore predict_server.py and train_and_serve.py

The refactor in #1902 has been merged, so the modification of the two files also committed in this pr.

global_args = vars(args)
print(global_args)
logging.info('init args: %s', global_args)
logging.info("init args: %s", global_args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use '' in train_and_serve.py, and use "" here? This file can use '' too.

Copy link
Contributor Author

@mangoGoForward mangoGoForward Jun 6, 2022

Choose a reason for hiding this comment

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

As the comment of pylint(https://github.com/PyCQA/pylint/blob/34c6aede493cfc72dcc94ae9085ab68b49e2960c/pylint/checkers/strings.py#L647-L651), the quote delimiters should used consistently throughout a module, may the rest of train_and_serve has more '', but the file of predict_server.py has more ', I think just keep consistent in whole file is ok.

Copy link
Collaborator

@vagetablechicken vagetablechicken Jun 7, 2022

Choose a reason for hiding this comment

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

Only ~200 lines, small file, use ' is ok. That's up to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

import gc
import os
import time
import glob
import requests
# fmt:off
import openmldb
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't delete import openmldb. It may get seg fault in some machine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@vagetablechicken vagetablechicken merged commit 3373f66 into 4paradigm:main Jun 8, 2022
@mangoGoForward mangoGoForward deleted the fix/demo-pylint-error branch June 8, 2022 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-standards code style/convention things docker openmldb compile image or demo image python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

python: pylint check found many error
3 participants