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

update code requirements, fix train.py #3

Closed
jorgeorpinel opened this issue Oct 31, 2019 · 17 comments · Fixed by #5
Closed

update code requirements, fix train.py #3

jorgeorpinel opened this issue Oct 31, 2019 · 17 comments · Fixed by #5
Assignees
Labels
bug Something isn't working dependencies Pull requests that update a dependency file

Comments

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Oct 31, 2019

From iterative/dvc.org#762 (comment)

Req's are pretty old:

tensorflow==1.11.0
keras==2.2.4
pillow==5.3.0

(but work. Last tested on Python 3.7)

UPDATE: Actually it's tensorflow==1.13.1 now, see #3 (comment).


Update https://dvc.org/doc/tutorials/versioning accordingly if this code changes.

@jorgeorpinel
Copy link
Contributor Author

@shcheklein said (in iterative/dvc.org#762 (comment)):

I think it's bette to review it and update either using PyTorch or the latest release TensorFlow + Keras.

@jorgeorpinel jorgeorpinel added the enhancement New feature or request label Oct 31, 2019
@casperdcl

This comment has been minimized.

@shcheklein

This comment has been minimized.

@casperdcl casperdcl self-assigned this Oct 31, 2019
@jorgeorpinel
Copy link
Contributor Author

p.s. sorry I was mistaken actually its tensorflow==1.13.1 now (1.11.0 is no longer available with pip) and also test.py is broken with:

Traceback (most recent call last):
  File "train.py", line 123, in <module>
    train_top_model()
  File "train.py", line 118, in train_top_model
    json.dump(history.history, open("metrics.json", 'w'))
...
  File "/.../lib/python3.7/json/encoder.py", line 179, in default
    raise TypeError(f'Object of type {o.__class__.__name__} '
TypeError: Object of type float32 is not JSON serializable

@jorgeorpinel jorgeorpinel changed the title update code requirements? update code requirements, fix train.py Oct 31, 2019
@jorgeorpinel jorgeorpinel added bug Something isn't working dependencies Pull requests that update a dependency file and removed enhancement New feature or request question Further information is requested labels Oct 31, 2019
@jorgeorpinel
Copy link
Contributor Author

Still interested given this ^ @casperdcl?

@casperdcl
Copy link
Contributor

yes easy fix just don't have much time over the next week or so

@jorgeorpinel
Copy link
Contributor Author

OK great, please self-assign when you have the time if this is still open. 🙂

@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Oct 31, 2019

Actually this is kind of a high priority I would say don't you think @shcheklein? Since basically our versioning tutorial is broken because of this.

Can someone from @iterative/engineering take a look? I was able to run train.py successfully by accident at some point but I'm not sure which versions of what I was using.

p.s. Preferably someone with a GPU because testing this takes a while on regular computers.

@casperdcl casperdcl self-assigned this Nov 1, 2019
@casperdcl
Copy link
Contributor

ah fine... there are a few other minor issues too. Will PR soon.

@casperdcl casperdcl mentioned this issue Nov 1, 2019
6 tasks
@casperdcl
Copy link
Contributor

ok see #5... btw I used tensorflow==2 and it takes less than 10 secs on my computer. We'd have to write tensorflow-gpu==2 in requirements.txt for a GPU version.

@jorgeorpinel
Copy link
Contributor Author

Thanks @casperdcl didn't mean to turn on the pressure! Just need to get this resolved soon by our team. Your contribution is greatly appreciated! Checking your PR... (will take some time to run on my MacBook air...)

@shcheklein
Copy link
Member

@jorgeorpinel @casperdcl I would avoid GPU - it can complicate installation. It should be quick enough when you run it even on CPU since model is pre-trained as far as I remember.

@casperdcl
Copy link
Contributor

yes AFAIK best practice is (seemingly as with dvc in this case) is to leave tensorflow out of requirements.txt completely and write in the docs that people need to install it themselves (so they can choose their own flavour).

@shcheklein
Copy link
Member

@casperdcl true. in this case I decided to specify it anyway since it's a simple example and we use virtualenv to completely isolate it Just for the sake of simplicity.

@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Nov 1, 2019

I would avoid GPU...

@shcheklein I just meant that train.py is not very fast. It's not super slow either but it takes around 10 min to run on my MacBook Air (when/if it works).

@casperdcl
Copy link
Contributor

casperdcl commented Nov 1, 2019

10 min? On my 12 core (48 with hyperthreading) machine with many background tasks running I still get under 10 secs. My guess is I/O rather than CPU is your main bottleneck?

@jorgeorpinel
Copy link
Contributor Author

I'm on a 2015 MacBook Air so maybe that explains it. But yeah maybe I/O. Who knows these things haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants