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

added option to use tensorflow.keras #123

Merged
merged 2 commits into from
Apr 14, 2022
Merged

Conversation

beasteers
Copy link
Contributor

@beasteers beasteers commented Sep 10, 2019

What does this implement/fix? Explain your changes.

I added a method to build tensorflow.keras from a pumpp.

import tensorflow.keras.layers as tfkl
...
input_ = pump.layers('tf.keras')[INPUT_NAME]

@bmcfee
Copy link
Owner

bmcfee commented Oct 8, 2019

Thanks for this one -- I don't think we should / need to invoke environment variables though, since it makes the internal behavior rather obscure and hard to predict. It'd be better if people just change their code to use the appropriate API for their use case.

@beasteers
Copy link
Contributor Author

Yeah I realized that and overwrote those changes (I think you're looking at an old version of the PR, I force pushed and updated the description a few weeks back)

Instead I just added pump.layers('tf.keras') which fits neatly in how things are currently done. Sorry about the confusion

@beasteers
Copy link
Contributor Author

Not sure how that old commit slipped by - you were right. Fixed now

@beasteers
Copy link
Contributor Author

Just thought I'd circle around on this one. It's a small change that adds pump.layers('tf.keras') as an input creation method - I fixed it so it's using the same method you use for creating keras vs tensorflow layers.

@amilkh
Copy link

amilkh commented May 20, 2020

Any update on when this will be merged?
https://pypi.org/project/vggish-keras/

@beasteers
Copy link
Contributor Author

That depends on Brian - he's a pretty busy person so I'm not expecting it soon. But seeing as this PR is still up to date with master, it should be safe for now.

I was working on that package a while back and it seems that for some reason I didn't add this specific branch as an install dependency.

In the most up to date version (0.1.1), it will install pumpp from this branch.

I also cleaned a couple things up and modified the API and README a bit to make things a bit more organized and clearer. So use the readme examples to update existing code (main change is that get_embedding and friends return the timestamps along with the embeddings.)

@bmcfee
Copy link
Owner

bmcfee commented May 26, 2020

I can probably get back to this project in the next couple of weeks. Got a few other things on my plate right now, but this PR generally looks good. I'll need to get my head back into pumpp for a minute to think it through, but it shouldn't be too long until we merge this. (Maybe a mid-june release?)

@bmcfee
Copy link
Owner

bmcfee commented Apr 14, 2022

This build is failing because of the following line, which wasn't tested in main 😱 :

pumpp/pumpp/feature/base.py

Lines 126 to 128 in 4009672

def layers_tensorflow(self):
from tensorflow import placeholder

The problem is that in bumping up to tf2, we lose the placeholder api. We can get it back by using v1 compat, eg:

import tensorflow.compat.v1 as tf1
tf1.placeholder(...)

The problem then becomes that this is incompatible with eager execution mode. We can disable this using tf1.disable_eager_execution(), but this produces a side effect that I'm not entirely happy about. AFAIK the recommended solution here (in tf2) is actually to use the keras interface (which works anyway) so I'm not too bothered by it, but it's worth pointing out.

Additionally, we might want to rename the tf options to "tf1" and "tensorflow1" for the 0.6 release.

@bmcfee bmcfee removed this from the 0.6.0 milestone Apr 14, 2022
@bmcfee bmcfee merged commit 8db32e7 into bmcfee:main Apr 14, 2022
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.

3 participants