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

fix: clarify py_modules usage in 1546 #1597

Merged
merged 6 commits into from Jan 5, 2021
Merged

fix: clarify py_modules usage in 1546 #1597

merged 6 commits into from Jan 5, 2021

Conversation

hanxiao
Copy link
Member

@hanxiao hanxiao commented Jan 5, 2021 •
edited

@hanxiao hanxiao requested a review from a team as a code owner January 5, 2021 16:46
@github-actions
Copy link

github-actions bot commented Jan 5, 2021

This PR closes: #1546

@jina-bot jina-bot added size/M area/core This issue/PR affects the core codebase area/helper This issue/PR affects the helper functionality area/testing This issue/PR affects testing labels Jan 5, 2021
@JoanFM
Copy link
Member

JoanFM commented Jan 5, 2021 •
edited

Should the title refer to 1546 and not 1549?

What is the learnings from here? Which patterns can be imported and which can't?

@hanxiao
Copy link
Member Author

hanxiao commented Jan 5, 2021

What is the learnings from here? Which patterns can be imported and which can't?

in test docstring

@hanxiao hanxiao changed the title fix: clarify py_modules usage in 1549 fix: clarify py_modules usage in 1546 Jan 5, 2021
This was linked to issues Jan 5, 2021
Multiple modules/files for custom executor #1546
Closed
error message only stated that my driver couldn't be found. But, in reality the whole py_module hasn't been loaded due to the import-error. #1480
Closed
@github-actions
Copy link

github-actions bot commented Jan 5, 2021 •
edited

Latency summary

Current PR yields:

  • 😶 index QPS at 1685, delta to last 3 avg.: -4%
  • 😶 query QPS at 32, delta to last 3 avg.: -4%

Breakdown

Version Index QPS Query QPS
current 1685 32
0.9.1 1735 33
0.8.22 1778 33
0.8.21 1783 33

Backed by latency-tracking. Further commits will update this comment.

JoanFM
JoanFM reviewed Jan 5, 2021
View reviewed changes
jina/importer.py
@@ -277,7 +277,12 @@ def _path_import(absolute_path: str) -> Optional[ModuleType]:
spec = importlib.util.spec_from_file_location('jinahub', absolute_path)
module = importlib.util.module_from_spec(spec)
user_module_name = os.path.splitext(os.path.basename(absolute_path))[0]
sys.modules[f'{spec.name}.{user_module_name}'] = module
if user_module_name == '__init__':
Copy link
Member

Choose a reason for hiding this comment

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

this may put some problems in many hub executors right?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, irrelevant, magic is in spec.loader.exec_module(module)

@jina-bot jina-bot added area/cicd This issue/PR affects the cicd pipeline area/housekeeping This issue/PR is housekeeping labels Jan 5, 2021
@codecov
Copy link

codecov bot commented Jan 5, 2021 •
edited

Codecov Report

Merging #1597 (768cf22) into master (93024f7) will increase coverage by 0.97%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1597      +/-   ##
==========================================
+ Coverage   83.86%   84.84%   +0.97%     
==========================================
  Files         128      128              
  Lines        6664     6725      +61     
==========================================
+ Hits         5589     5706     +117     
+ Misses       1075     1019      -56
Impacted Files Coverage Δ
jina/importer.py 65.08% <100.00%> (+1.48%) ⬆️
jina/logging/profile.py 69.84% <0.00%> (-0.56%) ⬇️
jina/drivers/craft.py 100.00% <0.00%> (ø)
jina/types/ndarray/generic.py 100.00% <0.00%> (ø)
jina/drivers/encode.py 94.91% <0.00%> (+0.08%) ⬆️
jina/enums.py 96.59% <0.00%> (+0.09%) ⬆️
jina/jaml/__init__.py 95.93% <0.00%> (+0.09%) ⬆️
jina/flow/base.py 87.08% <0.00%> (+0.09%) ⬆️
jina/clients/helper.py 94.00% <0.00%> (+0.12%) ⬆️
jina/peapods/runtimes/asyncio/rest/__init__.py 93.18% <0.00%> (+0.15%) ⬆️
... and 15 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 93024f7...3a2da3a. Read the comment docs.

@hanxiao hanxiao merged commit 8f4bc22 into master Jan 5, 2021
@hanxiao hanxiao deleted the fix-1546 branch January 5, 2021 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoanFM JoanFM JoanFM left review comments

@maximilianwerk maximilianwerk Awaiting requested review from maximilianwerk

@rutujasurve94 rutujasurve94 Awaiting requested review from rutujasurve94

Assignees
No one assigned
Labels
area/cicd This issue/PR affects the cicd pipeline area/core This issue/PR affects the core codebase area/helper This issue/PR affects the helper functionality area/housekeeping This issue/PR is housekeeping area/testing This issue/PR affects testing component/executor executor/meta size/M
Projects
None yet
Milestone
No milestone
3 participants
@hanxiao @JoanFM @jina-bot