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

feat(windows): beta support #3290

Merged
merged 39 commits into from
Sep 26, 2021
Merged

feat(windows): beta support #3290

merged 39 commits into from
Sep 26, 2021

Conversation

deepankarm
Copy link
Contributor

@deepankarm deepankarm commented Aug 30, 2021

@deepankarm deepankarm requested review from hanxiao and a team as code owners August 30, 2021 09:23
@github-actions github-actions bot added size/M area/core This issue/PR affects the core codebase area/entrypoint This issue/PR affects the entrypoint codebase area/helper This issue/PR affects the helper functionality area/network This issue/PR affects network functionality component/logging component/peapod component/type labels Aug 30, 2021
@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #3290 (806dcd7) into master (39242dd) will increase coverage by 0.92%.
The diff coverage is 83.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3290      +/-   ##
==========================================
+ Coverage   89.35%   90.28%   +0.92%     
==========================================
  Files         151      151              
  Lines       11014    11047      +33     
==========================================
+ Hits         9842     9974     +132     
+ Misses       1172     1073      -99     
Flag Coverage Δ
daemon 45.70% <50.84%> (-0.05%) ⬇️
jina 90.25% <83.05%> (+0.94%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jina/hubble/hubio.py 87.50% <0.00%> (-0.28%) ⬇️
jina/__init__.py 71.25% <66.66%> (+0.36%) ⬆️
jina/logging/profile.py 98.18% <66.66%> (-0.59%) ⬇️
jina/helper.py 83.12% <73.33%> (+1.23%) ⬆️
jina/logging/logger.py 92.63% <83.33%> (-0.85%) ⬇️
jina/types/arrays/memmap.py 95.03% <87.50%> (-0.23%) ⬇️
jina/flow/base.py 90.03% <100.00%> (+1.55%) ⬆️
jina/helloworld/multimodal/app.py 86.79% <100.00%> (+0.51%) ⬆️
jina/importer.py 87.17% <100.00%> (ø)
jina/peapods/peas/__init__.py 90.38% <100.00%> (+4.48%) ⬆️
... and 25 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 97519df...806dcd7. Read the comment docs.

@deepankarm deepankarm marked this pull request as draft August 30, 2021 09:32
@github-actions
Copy link

github-actions bot commented Aug 30, 2021

Latency summary

Current PR yields:

  • 🐎🐎🐎🐎 index QPS at 1194, delta to last 2 avg.: +11%
  • 🐎🐎 query QPS at 48, delta to last 2 avg.: +8%
  • 🐎🐎 dam extend QPS at 44887, delta to last 2 avg.: +12%
  • 🐎🐎 avg flow time within 1.6038 seconds, delta to last 2 avg.: -4%
  • 🐢🐢 import jina within 0.3859 seconds, delta to last 2 avg.: -12%

Breakdown

Version Index QPS Query QPS DAM Extend QPS Avg Flow Time (s) Import Time (s)
current 1194 48 44887 1.6038 0.3859
2.1.3 1108 46 42990 1.6358 0.4305
2.1.2 1027 42 36736 1.7097 0.4542

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

@@ -7,19 +7,19 @@ executors:
metas:
workspace: $HW_WORKDIR
py_modules:
- my_executors.py
- $APP_DIR/my_executors.py
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done to solve the issue with yaml register_cls / pymodule imports in a spawned process. Should be reverted

@github-actions github-actions bot added area/cicd This issue/PR affects the cicd pipeline area/housekeeping This issue/PR is housekeeping labels Aug 30, 2021
@github-actions github-actions bot added area/setup This issue/PR affects setting up Jina component/resource labels Aug 30, 2021
@deepankarm deepankarm force-pushed the feat-windows-beta branch 6 times, most recently from 5ae4313 to 21f8926 Compare September 21, 2021 10:17
@deepankarm deepankarm force-pushed the feat-windows-beta branch 2 times, most recently from 3fafb3b to c28432b Compare September 21, 2021 12:40
@deepankarm deepankarm changed the title feat: windows support (alpha) feat(windows): beta support Sep 21, 2021
@deepankarm deepankarm marked this pull request as ready for review September 21, 2021 15:32
@deepankarm deepankarm requested a review from alexcg1 as a code owner September 21, 2021 15:32
@@ -17,6 +17,9 @@

import numpy as np

from ... import __windows__

__windows__ = True
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alaeddine-13 this is left from debugging?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, will clean it

Comment on lines +290 to +291
if __windows__:
self._body.seek(self._start)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, actually mmap seeks _body to 0 on windows

@@ -42,7 +44,7 @@ def _warning_on_one_line(message, category, filename, lineno, *args, **kwargs):

_set_start_method(_start_method.lower())
_warnings.warn(f'multiprocessing start method is set to `{_start_method.lower()}`')
_os.unsetenv('JINA_MP_START_METHOD')
_os.environ.pop('JINA_MP_START_METHOD')
Copy link
Member

@hanxiao hanxiao Sep 25, 2021

Choose a reason for hiding this comment

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

but is this really the same as unsetenv? isn't dict.pop modify the dict object itself instead of really unset env vars?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to do the same thing as unsetenv (tested on Ubuntu, python 3.7.11)

import os
import subprocess

echo = lambda x, y: print(x, '\t', subprocess.check_output([f'echo ${y}'], shell=True, universal_newlines=True).strip())

def _pop():
    var_name = 'FOO_POP'
    os.environ[var_name] = 'BAR'
    echo('after set env: ', var_name)
    os.environ.pop(var_name)
    echo('after os.environ.pop: ', var_name)

def _unsetenv():
    var_name = 'FOO_UNSET'
    os.environ[var_name] = 'BAR'
    echo('after set env: ', var_name)
    os.unsetenv(var_name)
    echo('after os.unsetenv: ', var_name)

def _del():
    var_name = 'FOO_DEL'
    os.environ[var_name] = 'BAR'
    echo('after set env: ', var_name)
    del os.environ[var_name]
    echo('after del os.environ: ', var_name)

_pop()
_unsetenv()
_del()
after set env:           BAR
after os.environ.pop:    
after set env:           BAR
after os.unsetenv:       
after set env:           BAR
after del os.environ: 

Copy link
Member

Choose a reason for hiding this comment

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

interesting! thanks for explaining it

@@ -1036,7 +1036,7 @@ def __exit__(self, exc_type, exc_val, exc_tb):
# unset all envs to avoid any side-effect
if self.args.env:
for k in self.args.env.keys():
os.unsetenv(k)
os.environ.pop(k, None)
Copy link
Member

Choose a reason for hiding this comment

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

same here as above

Affects:
- Either, change your code from using `from {user_module_name} import ...`
- Either, change your code from using `from {user_module_name} import ...`
Copy link
Member

Choose a reason for hiding this comment

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

out of the scope of this PR, but im not really sure this warning is still valid @tadejsv

Copy link
Member

@hanxiao hanxiao left a comment

Choose a reason for hiding this comment

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

some minor changes

@github-actions
Copy link

📝 Docs are deployed on https://feat-windows-beta--jina-docs.netlify.app 🎉

@hanxiao hanxiao merged commit 2909933 into master Sep 26, 2021
@hanxiao hanxiao deleted the feat-windows-beta branch September 26, 2021 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cicd This issue/PR affects the cicd pipeline area/core This issue/PR affects the core codebase area/docs This issue/PR affects the docs area/entrypoint This issue/PR affects the entrypoint codebase area/helper This issue/PR affects the helper functionality area/housekeeping This issue/PR is housekeeping area/network This issue/PR affects network functionality area/setup This issue/PR affects setting up Jina area/testing This issue/PR affects testing component/flow component/logging component/peapod component/type size/L size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add native Windows support
6 participants