SDK - Preventing errors when importing kfp.notebook

[Ark-kun]

This change is 

[gaoning777]

This function is not covered by any tests. Can we add a test for it?

[Ark-kun]

This function is not covered by any tests. Can we add a test for it?

Hmm. I guess we should test it using sample test the same way we test image building functions. I think we have a sample notebook showing this function. I'll check whether it's covered by sample tests.

[gaoning777]

Thanks.

[Ark-kun]

This function is not covered by any tests. Can we add a test for it?

I've checked and this magic is used in a sample notebook which is covered by sample test.

[animeshsingh]

@Ark-kun would like to understand more about this functionality - what`s the scope?

[Ark-kun]

@Ark-kun would like to understand more about this functionality - what`s the scope?

%%docker is just a very thin syntactic sugar over the build_docker_image function.
It can be used from a notebook.

This PR just stops the file from crashing programs that try to import the package.

[gaoning777]

/lgtm

[gaoning777]

Could you add a user-friendly message in case of import failure?

[Ark-kun]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• sdk/OWNERS [Ark-kun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• sdk/OWNERS [Ark-kun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Ark-kun]

Could you add a user-friendly message in case of import failure?

Not having the IPython instance running is not an error now, so there is no error message.

[gaoning777]

I mean user-friendly message that there is no magic docker() if there is no IPython.

[Ark-kun]

I mean user-friendly message that there is no magic docker() if there is no IPython.

I'm not sure how that would work. Can you give a scenario?

With this fix, the docker function is always available and working.

To be able use a magic (whether a working one or the one raining errors) there needs to be a notebook/IPython running.

[gaoning777]

I meant:
try:
import IPython
docker = IPython.core.magic.register_cell_magic(docker)
except ImportError:
print "There is no IPython module, thus no magic docker command."
pass
except NameError:
pass

Something like this to warn the users about the inavailability of the docker command if the IPython is not found.

[Ark-kun]

warn the users about the inavailability of the docker command

But how would the users use that command if they though it's available?

%%docker is syntax error in Python.

I see no scenario whether the user can actually detect that "the %%docker magic is unavailable". It's Catch-22: The users can only detect the presence of magic when they're in the IPython environment. But in that case the magic is always registered.

[gaoning777]

Even though they would not be able to use the docker, it still helps show a message that if they want to use the docker magic, use it in the notebook.
My point is that people might import the notebook module without using the docker because they simply copy and paste it from the sample. The message will let them know the docker magic in the notebook.