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

Feature/Python3 & Windows10 compatibility #52

Closed
wants to merge 56 commits into from

Conversation

nklapste
Copy link

@nklapste nklapste commented Jan 9, 2020

Migrating code to work for Python3. This drops Python2 support.

Adding support to execute within a windows environment.

Fixing path specifications to have better support on windows.

Tested with a tensorflow==2.0.0 backend on a windows 10 machine.
Features:

  • Python3 compatibility
  • Windows 10 compatibility
  • tensorflow 2 compatibility
  • scripts/download_weights.py is now system agnostic by using requests
  • .travis.yml build now tests on windows 10, osx, and linux

TODO:

  • some changes regarding updates to keras and tensorflow might have changed behavior with using the prebuilt deepmoji_weights.hdf5 model
    • test test_finetuning.test_encode_texts is raising a AssertionError as its avg_across_sentances is [-0.024 0.027 -0.046 -0.002 -0. ] instead of test's expected [-0.023, 0.021, -0.037, -0.001, -0.005]
  • code within examples has not been heavily tested or validated on windows 10
  • verify scripts/analyze_all_results.py
  • verify scripts/analyze_results.py

@bfelbo
Copy link
Owner

bfelbo commented Jan 10, 2020

Amazing, thank you for working on this! 🙌

@nklapste
Copy link
Author

@bfelbo are you comfortable with dropping Python2.7 support? Maybe locking the python2.7 version in another branch or tag?

Otherwise, I could look into making this cross-compatible with python2.7 and python3+, but, in my experience this leads to messy code.

@bfelbo
Copy link
Owner

bfelbo commented Jan 10, 2020

Completely agree. Let’s drop 2.7 and make a new tag/release.

Similarly, I’ve been thinking of upgrading the code to TF2 and dropping support for TF1. It requires converting the pretrained model to TF2, but that hopefully shouldn’t be too hard.

@bfelbo
Copy link
Owner

bfelbo commented Jan 10, 2020

Feel free to add a line to the README that you added Python3 support :)

@nklapste
Copy link
Author

@bfelbo For verifying scripts/analyze_all_results.py and scripts/analyze_results.py both require <dataset>_<method>_results.txt files. I sadly cannot find those files. Are these generated somehow or located in the repo?

@nklapste
Copy link
Author

Besides some refactoring, this PR should be ready for review.

However the following scripts could not be verified to work by me scripts/analyze_all_results.py
and scripts/analyze_results.py

@nklapste nklapste changed the title WIP: Feature/python3 window10 Feature/python3 window10 Jan 13, 2020
@nklapste nklapste changed the title Feature/python3 window10 Feature/Python3 Windows10 compatibility Jan 13, 2020
@nklapste nklapste changed the title Feature/Python3 Windows10 compatibility Feature/Python3 & Windows10 compatibility Jan 13, 2020
@nklapste nklapste requested a review from bfelbo January 16, 2020 15:32
@bfelbo
Copy link
Owner

bfelbo commented Jan 16, 2020

Thank you Nathan. I'll take a look on the weekend :)

@bfelbo
Copy link
Owner

bfelbo commented Jan 19, 2020

Looking at it now. There's a lot of changes so it might take some time.

@bfelbo
Copy link
Owner

bfelbo commented Jan 19, 2020

Thanks for this PR. It's clear that you've put a lot of effort into this.

I've been trying to review this, but it's hard as there's a lot of other changes to the code besides adding Python3 / Windows compatibility. A few examples:

  • Travis setup
  • .gitignore changes
  • New get_vocabulary() function, which is used in all examples
  • Linting changes (import order, indenting, line breaks)
  • Removing the os.path logic to simplify where the code can be called from

Some of these changes might be useful, but it'd be great if they could be submitted as separate pull requests after moving the codebase to Python3. The smaller the pull request, the easier it is to review/merge it :)

Can you update this PR (or create a new one) to contain only the minimal changes needed for Python3 support?

That'd be amazing.

@bfelbo
Copy link
Owner

bfelbo commented Jan 19, 2020

If I try to run python3 score_texts_emojis.py with the code in this PR, I get the error ModuleNotFoundError: No module named 'create_vocab'. If you remove all the non-essential changes from this PR, then it'll also be less likely to break the code :)

@nklapste
Copy link
Author

I'll minify this PR to is base elements! Thanks for your initial reviews!

was opening writing as bytes when content to be written was `str`
@nklapste
Copy link
Author

If I try to run python3 score_texts_emojis.py with the code in this PR, I get the error ModuleNotFoundError: No module named 'create_vocab'. If you remove all the non-essential changes from this PR, then it'll also be less likely to break the code :)

Have you installed the deepmoji project locally?

Running the following command:

pip install . 

Within the project directory should install deepmoji to your python environment. Afterwards, you should be able to import deepmoji.* modules without issues.

Within my installed deepmoji environment I'm able to run python score_texts_emojis.py.

@nklapste
Copy link
Author

PR continued on with #53 closing this bloated PR

@nklapste nklapste closed this Jan 20, 2020
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.

2 participants