-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[MO] pip packaging #3123
[MO] pip packaging #3123
Conversation
976928d
to
a68e5ad
Compare
@lazarevevgeny, @mryzhov, please review updated version of PR |
@dkurt Can you please describe how user should use this package?
|
See example here: deepchem/deepchem#2332. We need this usage to use MO from GitHub as pip dependency.
Not clear for me what kind of documentations should be added?
There were a proposal at initial commit 6a663f3. But we need this feature ASAP so don't want to block on API review. |
def find_package_modules(self, package, package_dir): | ||
modules = super().find_package_modules(package, package_dir) | ||
return [ | ||
(pkg, module, filename) | ||
for (pkg, module, filename) in modules | ||
if not filename.endswith('_test.py') | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse the CMake script which is already available for the MO to get list of files which needes to be included into the package?
CC @slyubimt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that CMake dependency is a good option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a script "/automation/create_package.py" which will create an archive with all necessary files. Instead of implementing logic to determine list of files to be included in the wheel we can re-use this script.
path = os.path.join(self.install_purelib, package_name, '__init__.py') | ||
with open(path, 'wt') as f: | ||
f.write('import os, sys\n') | ||
f.write('from {} import mo\n'.format(package_name)) | ||
# This is required to fix internal imports | ||
f.write('sys.path.append(os.path.dirname(__file__))\n') | ||
# We install a package into custom folder "package_name". | ||
# Redirect import to model-optimizer/mo/__init__.py | ||
f.write('sys.modules["mo"] = mo') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change something in the MO so we could get rid of this workaround?
I got your point, sorry for confusion. I'll try to fix that. UPD: Solved |
What I mean here is that wrapper which you are making at https://github.com/deepchem/deepchem/pull/2332/files#diff-26fde86013198b5c8d4604dae3d3bd4ed19c9aa18de4db58a4b83dd786c6613fR128 calls MO as it's already installed and configured for tf usage, but MO requires installation of requirements_tf.txt or requirements_tf2.txt before it can be used. |
@SDxKeeper, I fixed this problem so no |
@dkurt thank you for removing venv/requirements*.txt files. Sorry I didn't notice update on comment 9 days ago. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkurt could you please rebase PR to rerun the tests
Shall we release a wheel (maybe named openvino-mo) on pypi.org? |
@clouds56, there is already |
@dkurt thanks for the information! That's exactly what I need. |
Usage
Install
or by wheel
Key features:
*_test.py
andtest-generator
)