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

TF2 Upgrader: add support of ipynb files #25680

Merged
merged 6 commits into from Feb 25, 2019

Conversation

@lc0
Copy link
Contributor

commented Feb 11, 2019

As discussed @martinwicke, since locally it was tested in isolated environment, I would be more than happy to run the required tests

Potentially I'd follow with updating readme and other docs related things

@googlebot googlebot added the cla: yes label Feb 11, 2019

@lc0

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

I think once, #25653 is in, I can rebase this one

@martinwicke
Copy link
Member

left a comment

This is super-awesome!



def main():
parser = argparse.ArgumentParser(
formatter_class=argparse.RawDescriptionHelpFormatter,
description="""Convert a TensorFlow Python file to 2.0
description="""Convert a TensorFlow Python file from 1.* to 2.0

This comment has been minimized.

Copy link
@martinwicke

martinwicke Feb 12, 2019

Member

I prefer 1.x

This comment has been minimized.

Copy link
@lc0

lc0 Feb 12, 2019

Author Contributor

funny, since I started as 1.x, but in a header, it was already written as 1.*

@@ -85,7 +87,7 @@ def main():
raise ValueError(
"--outfile=<output file> argument is required when converting a "
"single file.")
files_processed, report_text, errors = upgrade.process_file(
files_processed, report_text, errors = ipynb.process_file(

This comment has been minimized.

Copy link
@martinwicke

martinwicke Feb 12, 2019

Member

I'd prefer the filename selection logic to live here.

This comment has been minimized.

Copy link
@martinwicke

martinwicke Feb 12, 2019

Member

Also, look at line 99, we should make sure that convert tree also understands ipynb files.

files_processed, report_text, errors = \
upgrader.process_file(in_filename, out_filename)

elif in_filename.endswith('.ipynb'):

This comment has been minimized.

Copy link
@martinwicke

martinwicke Feb 12, 2019

Member

Can you move the dispatch logic to the main function (and possibly maybe even process_tree)

@pragyaak pragyaak self-assigned this Feb 12, 2019

@pragyaak pragyaak added the size:M label Feb 12, 2019

@pragyaak pragyaak added this to Assigned Reviewer in PR Queue via automation Feb 12, 2019

@lc0

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

@martinwicke Thanks for review, I've updated accordingly.
And with tree part, I would add it as a second PR potentially.

@martinwicke
Copy link
Member

left a comment

LG, doing the tree stuff in a separate PR makes sense.

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Feb 13, 2019

@pragyaak pragyaak assigned rthadur and unassigned pragyaak Feb 23, 2019

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Feb 23, 2019

@lc0 lc0 force-pushed the lc0:tf2-ipynb-feature branch from 7a733d1 to c9ee938 Feb 23, 2019

@lc0

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

@martinwicke I fixed linter and other build issues. Please let me know if anything additional is missing.

@tensorflow-copybara tensorflow-copybara merged commit 424f6c9 into tensorflow:master Feb 25, 2019

15 of 17 checks passed

Windows Bazel GPU Internal CI infrastructure error. Please apply label 'kokoro:force-run' to rerun the build.
Details
Windows Bazel Internal CI build failed
Details
Android Demo App Internal CI build successful
Details
GPU CC Internal CI build successful
Details
GPU Python3 Internal CI build successful
Details
MacOS Contrib Internal CI build successful
Details
MacOS Python2 and CC Internal CI build successful
Details
Ubuntu CC Internal CI build successful
Details
Ubuntu Makefile Internal CI build successful
Details
Ubuntu Python2 Internal CI build successful
Details
Ubuntu Python3 Internal CI build successful
Details
Ubuntu Python3 PIP Internal CI build successful
Details
Ubuntu Sanity Internal CI build successful
Details
Ubuntu contrib Internal CI build successful
Details
XLA Internal CI build successful
Details
cla/google All necessary CLAs are signed
import/copybara Change imported to the internal review system
Details

PR Queue automation moved this from Reviewer Requested Changes to Merged Feb 25, 2019

tensorflow-copybara pushed a commit that referenced this pull request Feb 25, 2019

Merge pull request #25680 from lc0:tf2-ipynb-feature
PiperOrigin-RevId: 235546488
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
9 participants
You can’t perform that action at this time.