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

Fix typos on intro to keras for engineers #21

Merged
merged 1 commit into from
May 11, 2020
Merged

Fix typos on intro to keras for engineers #21

merged 1 commit into from
May 11, 2020

Conversation

pedro823
Copy link
Contributor

Summary

Fixed typos on the template.

However, I wasn't able to run the autogen to generate all the typo fixes on all the versions.

Does the newly introduced CI do that? If so, we need to change the documentation on autogeneration as well.

Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

@fchollet fchollet merged commit 0fb3b02 into keras-team:master May 11, 2020
@fchollet
Copy link
Member

Curious why you couldn't run the command?

@pedro823
Copy link
Contributor Author

@fchollet, my steps were as follows:

  1. Created a virtualenv and got a shell over there.
  2. Got the list of dependencies that were declared in the header of autogen.py and installed them:
pip install pygments jinja2 markdown requests mdx_truly_sane_lists
  1. Running it, I get an error of no module named sphinx.
ModuleNotFoundError: No module named 'sphinx'
  1. So I installed it with pip install sphinx, and ran it again. There's the same error with the black module, so I installed it as well.

  2. At this point, python3 autogen.py make starts to do some output. However, at api/models we find another missing module, tensorflow:

...Processing api/models
Traceback (most recent call last):
  File "/Users/raz/web/keras-io/scripts/docstrings.py", line 113, in import_object
    last_object_got = importlib.import_module(".".join(seen_names))
  File "/Users/raz/web/keras-io/venv/lib/python3.7/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 965, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'tensorflow'

Installed it, next.

  1. This is when I hit a wall: running it again, it stopped on api/preprocessing, which needs more than just module installation.
...Processing api/preprocessing
Traceback (most recent call last):
  File "/Users/raz/web/keras-io/scripts/docstrings.py", line 113, in import_object
    last_object_got = importlib.import_module(".".join(seen_names))
  File "/Users/raz/web/keras-io/venv/lib/python3.7/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 965, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'tensorflow.keras.preprocessing.image_dataset_from_directory'

Installing keras didn't help.

There are some follow ups to this adventure:

  • The process of installation could probably use a Pipfile from pipenv. Installation could be as easy as a pipenv sync.
  • The process of generating the files should be separated from the process of checking if the code of the files is correct. I didn't stop to read the code, but I assume that's it, since normally you wouldn't need tensorflow & keras to build a static page.
  • With that made, we could even go a step further and make the CI verify that autogen.py was run before merging the PR by adding a python autogen.py verify!

Can I create an issue and start working on those things? :)

@pedro823 pedro823 deleted the pedro823/fix-typos branch May 11, 2020 19:16
@fchollet
Copy link
Member

fchollet commented May 12, 2020

We can add a requirements.txt to fix these issues. tf-nightly is required.

Also, in your case you edited a .md file, but that file is supposed to be autogenerated. You should only edit the .py files. I resubmitted a PR copying the fixes in the .py file.

@pedro823
Copy link
Contributor Author

@fchollet do you think that should be on a README.md or a CONTRIBUTING.md? That'd help more newcomers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants