Review Items

.project-metadata.yaml

• It'd be good to keep an Engines specification in addition to the runtimes spec. This way users who do not have runtimes for whatever reason aren't left guessing. Can we include an engine spec here after the runtime spec? (see any of the other released AMPs to copy/paste from)
• The project is currently failing during the "Download Data" job. Looks like we're missing a package. I see that we are installing requirements in-line in the notebook, so this dependency is missing when the download is attempted. Might make sense to add a dedicated "Install Dependencies" step, or combine the install and download into one? Also, FYI this can be run as a Session task instead of a Job for simplification - see this example for reference
  
• Side note: I notice that the runtime specified here is edition: Standard. Have you tried using a GPU runtime? Would it make sense to recommend that here?
  • After writing this, I noticed the GPU callout in the README... out of curiosity, what issue was faced with GPUs?
• I believe the kernel parameter is incorrectly specified in the Job. My hunch is that this will fail if run on an Engine, but I haven't tested to be sure. See this example or the Cloudera AMP docs to confirm the correct syntax.

READMEmd

• Should we link to the first published blog post from the README as well as in the notebook?

video_classification.ipynb

• This link goes to a private Notion page instead of the public FFL blog:
  
• Looks like this function is throwing an IndexError because the path to the data is one that is local to your dev environment. Changing the path to /home/cdsw/data/raw/kinetics/400/val/*/ worked for me.
  
  
• With the data path corrected, it looks like the video's themselves still aren't displaying/playing in the notebook? I'm guessing this has something to do with IPython Display in notebook vs. lab... maybe?
  
• We can now fill in this placeholder link:
  
• During the evaluation section of the notebook, it appears that some of the files do not actually exist in the 400 that were downloaded initially? Could this error exist because previously we were referencing a local file path issue mentioned above (that may have had a larger subset of files)?
  

Other Comments

• I LOVE the FancyArgumentParser 💥
• Also LOVE the KineticsLoader class 💪
• Also also LOVE the use of type hinting all over the place. I'm inspired. 🌟
• Is it reasonable to demand a 16GB session to run this AMP? Maybe we could process 4 videos instead of 8 and cut the RAM requirement in half? Maybe Juno has an opinion on this?
• Super nitpicky...but I somehow noticed that the Cloudera source header in setup.py has year == 2020, while all the others are 2021. I'm fine with keeping them all 2021, but all should be consistent probs.
• Can you share an image you'd like to use for the thumbnail in the review doc?
• Can you review/update the AMP tile metadata in the tracking doc (final repo name, short/long description, etc.)?