AMP Review 2

[andrewrreed]

.project-metadata.yaml

• [x] Is there a need to specify a runtime here? I don't think there's any harm in doing so, except it might possibly confuse users who see that and also see the README instructions to run a Python3 Jupyter notebook session.
  • Also, just checked the other "notebook" AMPs and they omit any type of runtime specification... so might be best to remain consistent? @cjwallace might have thoughts/preference on this?
  • Side Note - it would probably be cool to have a new AMP feature that allowed us to pass an argument like "persist: True in the yaml that allowed us to start up and continue running a Jupyter session for the end user. That way when they deploy the AMP, the notebook is waiting for them... 🤔

train_gensim_word2vec.ipynb

• [x] minor typo
• [x] missing link
• [x] Minor typo

README.md

• [x] need to remove placeholder
  • for the record, 2vCPU/4GB ran just fine for me!
  • might also be worth mentioning the desired Python version this was developed against (3.8?)
• [x] I think this title is different from whats in the tracking doc - whichever way you prefer, they should probs match to avoid confusion

Copyright source headers

These dang things again! Just need to add them to the top of these modules:

• [ ] https://github.com/fastforwardlabs/train_embeddings_with_gensim/blob/master/recsys/data.py
• [ ] https://github.com/fastforwardlabs/train_embeddings_with_gensim/blob/master/recsys/metrics.py
• [ ] https://github.com/fastforwardlabs/train_embeddings_with_gensim/blob/master/recsys/models.py
• [ ] https://github.com/fastforwardlabs/train_embeddings_with_gensim/blob/master/recsys/utils.py

Misc.

• [x] Can you drop an image for the CML thumbnail in here for me? Thanks!

[andrewrreed]

@melaniebeck This is an awesome, densely-packed notebook! Really enjoyed working through this and learned quite a bit about Gensim and Ray because of it. A few minor fixes documented above, then should be good to go. Lemme know if you have any questions - thanks!

[cjwallace]

On the runtime image, my understanding is this AMP strictly requires Python >=3.8, and providing a runtime spec (+ maybe some words in the README) is the only way we can convey that, absent any real dependency management system.

[cjwallace]

We could change https://github.com/fastforwardlabs/train_embeddings_with_gensim/blob/2c94ca2b45cff18004142b25028373061983fa86/.project-metadata.yaml#L10 to JupyterLab, if a notebook is the main entry point to the project.

(edit: but it won't work until the next release of CML)