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

[MO] move importlib-metadata into setup.py #10319

Conversation

pavel-esir
Copy link
Contributor

@pavel-esir pavel-esir commented Feb 11, 2022

Root cause analysis: during requirements check MO was not handling and marker when sys_platform and python_version are specified simultaneously, e.g. importlib-metadata; python_version < "3.8" and sys_platform == "win32" and shows warning:

[ WARNING ] The version checker doesn't support environment marker combination and it will be 
ignored: python_version < "3.8" and sys_platform == "win32"

Solution: Moved importlib-metadata from requirements.txt into setup.py

Ticket: CVS-77062

Code:

  • Comments
  • Code style (PEP8)
  • Transformation generates reshape-able IR
  • Transformation preserves original framework node names
  • Transformation preserves tensor names

Validation:

  • Unit tests: added new cases, existing tests didn't broke
  • n/a Framework operation tests - no new operations were added
  • n/a Transformation tests - no new transformation were added
  • n/a Model Optimizer IR Reader check

Documentation:

  • n/a Supported frameworks operations list
  • n/a Guide on how to convert the public model
  • n/a User guide update

@pavel-esir pavel-esir added bug Something isn't working category: MO Model Optimizer labels Feb 11, 2022
@pavel-esir pavel-esir added this to the 2022.1 milestone Feb 11, 2022
@pavel-esir pavel-esir requested review from achetver, a team and rkazants February 11, 2022 16:29
Copy link
Member

@rkazants rkazants left a comment

Choose a reason for hiding this comment

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

Why do we need importlib-metadata in requirements.txt file? no other tools use it. I propose to remove it and this change is not needed at all

@pavel-esir
Copy link
Contributor Author

Why do we need importlib-metadata in requirements.txt file? no other tools use it. I propose to remove it and this change is not needed at all

We need importlib-metadata for entry_points on windows. Without that package calling mo --input_model ... would not work.
It is needed only for windows and only if python version is < 3.7, on linux and py >= 3.8 entry_points work without this requirement.

We are willing to make this as default recommended path to use MO therefore we can not remove this from requirements. But if my changes look too risky we can make this package required for both windows and linux or/and for all versions of python. And close this PR. This should not cause any harm since the package is lightweight.

@pavel-esir pavel-esir requested a review from rkazants February 14, 2022 11:13
@rkazants
Copy link
Member

@pavel-esir, please remove importlib-metadata from requirements.txt and leave it only in setup.py as we agreed. No other changes are needed.

@pavel-esir
Copy link
Contributor Author

@pavel-esir, please remove importlib-metadata from requirements.txt and leave it only in setup.py as we agreed. No other changes are needed.

ok, i'll do this

@pavel-esir pavel-esir changed the title [MO] handle 'and' marker in requirements [MO] move importlib-metadata into setup.py Feb 14, 2022
@pavel-esir
Copy link
Contributor Author

Moved importlib-metadata into setup.py, @rkazants please take a look

Copy link
Contributor

@andrei-kochin andrei-kochin left a comment

Choose a reason for hiding this comment

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

LGTM

@andrei-kochin andrei-kochin enabled auto-merge (squash) February 14, 2022 17:07
@andrei-kochin andrei-kochin merged commit 121d59a into openvinotoolkit:master Feb 15, 2022
@pavel-esir pavel-esir deleted the handle_'and'_requirements_marker branch February 15, 2022 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working category: MO Model Optimizer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants