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

minimal check for import V3 #3175

Merged
merged 24 commits into from May 15, 2020

Conversation

guihao-liang
Copy link
Collaborator

@guihao-liang guihao-liang commented May 7, 2020

Branched from #3152.

As it is discussed with @hoytak, @TobyRoseman, and @nickjong in #3156, I provide a simpler version for minimal package lazy import in order to lower the maintenance cost.


update 07/05/2020

This PR aims to provide some instructions to users who install the minimal package but somehow wind up using some ML functionalities.

For example, minimal pkg doesn't have TF installed as its dependency. When a user uses ML toolkit that relies on TF, he/she should see the instruction to install the full package's dependency.

Traceback (most recent call last):
...
ModuleNotFoundError: No module named 'tensorflow'. This is a minimal package for SFrame only, without tensorflow pinned as a dependency. You can try to install all required packages by installing the full package by:
**pip install turicreate==6.2**

Instead of letting them install each package individually, let them install turicreate==VERSION to rely on the full package's requirement list to install all required dependencies.

@guihao-liang guihao-liang added this to the 6.3 milestone May 7, 2020
@guihao-liang guihao-liang self-assigned this May 7, 2020
@guihao-liang guihao-liang changed the title minimal check for import V2 minimal check for import V2 - the last dance May 7, 2020
@guihao-liang guihao-liang changed the title minimal check for import V2 - the last dance minimal check for import V3 May 7, 2020
# This toolkit is compatible with TensorFlow V2 behavior.
# However, until all toolkits are compatible, we must call `disable_v2_behavior()`.
_tf.disable_v2_behavior()
# in conjunction with minimal package
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! Let's move this function into _tf_utils as it's copy-pasted elsewhere, then there's only one of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there must be a reason for it to exist everywhere before. If we need to clean it, why now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't you add _lazy_import_tensorflow? It was added in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it's a function name to wrap the existing code.

Hoyt Koepke and others added 12 commits May 7, 2020 18:38
… are built as well.

Next attempt at valid setup.cfg file.

Got wheels to package correctly.

Update.

Okay, it all works now locally.

Pinned llvm version.

one minimal build
address pr concerns

address pr concerns

address pr concerns

further clean

add minimal_import in

merge disaster

add encoding string to this pr

remove coreml and resampy

restore some file
@guihao-liang
Copy link
Collaborator Author

for 3 files (drawing, activity, and style), at top-level, I wrap

    import tensorflow.compat.v1 as _tf
    # This toolkit is compatible with TensorFlow V2 behavior.
    # However, until all toolkits are compatible, we must call `disable_v2_behavior()`.
    _tf.disable_v2_behavior()

into,

def _lazy_import_tensorflow():
    _tf = minimal_package_import_check("tensorflow.compat.v1")
    # This toolkit is compatible with TensorFlow V2 behavior.
    # However, until all toolkits are compatible, we must call `disable_v2_behavior()`.
    _tf.disable_v2_behavior()
    return _tf

and only for the sound classifier, I wrap

    _utils.suppress_tensorflow_warnings()
    import tensorflow.compat.v1 as _tf
    # This toolkit is compatible with TensorFlow V2 behavior.
    # However, until all toolkits are compatible, we must call `disable_v2_behavior()`.
    _tf.disable_v2_behavior()

into

def _lazy_import_tensorflow():
    # Suppresses verbosity to only errors
    _utils.suppress_tensorflow_warnings()

    _tf = minimal_package_import_check("tensorflow.compat.v1")
    # This toolkit is compatible with TensorFlow V2 behavior.
    # However, until all toolkits are compatible, we must call `disable_v2_behavior()`.
    _tf.disable_v2_behavior()

    return _tf

These should not be considered as the duplicate of code because each tool may have its own settings on tf. Leaving it at the beginning of the file will allow people to know what's the tf configuration more clearly. I only add 2 more lines around. I don't believe that these 2 more lines are considered as the liability I shouldn't put here since people may not be able to maintain it because these 2 lines increase the mental burden on them so that they can't understand it.

Second, if this is considered as the duplicate of code, we wrap all common imports into on function to remove duplicate of import tensorflow orimportosorimport sys`.

@hoytak how do you think?

@guihao-liang
Copy link
Collaborator Author

passed internal tests (112066).

@guihao-liang
Copy link
Collaborator Author

guihao-liang commented May 8, 2020

from #3180:

core implementation for lazy import (including comments): 392 lines added
client adoption code: 48 lines added

from this PR:
core implementation for lazy import (including comments): 103 lines added
client adoption code: 165 lines added

The team favors later one is reasonable. Even more labor work and more client code affected, this PR comes with lower risk and liability.

# This toolkit is compatible with TensorFlow V2 behavior.
# However, until all toolkits are compatible, we must call `disable_v2_behavior()`.
_tf.disable_v2_behavior()
# in conjunction with minimal package
Copy link
Collaborator

Choose a reason for hiding this comment

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

Didn't you add _lazy_import_tensorflow? It was added in this PR.

@guihao-liang guihao-liang requested a review from hoytak May 12, 2020 15:03
@guihao-liang
Copy link
Collaborator Author

@hoytak @TobyRoseman updated in ed670ff, the version of turicreate is also modified to resolve the complaint that,

ERROR: <package> has requirement turicreate==6.1.1+minimal, but you'll have turicreate 6.1.1 which is incompatible.

@guihao-liang
Copy link
Collaborator Author

this 6d80701 will trim the "+minimal" from local version (the downstream version) to prompt the upstream version to user if they use any ML toolkits from the minimal version.

@hoytak
Copy link
Collaborator

hoytak commented May 15, 2020

Good work! Ready to go.

@guihao-liang guihao-liang merged commit 5cf5652 into apple:master May 15, 2020
@guihao-liang guihao-liang added this to Done in 6.3 via automation May 15, 2020
@guihao-liang guihao-liang deleted the 05-06-prompt-for-minimal branch May 15, 2020 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
6.3
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants