clamsproject / app-swt-detection

CLAMS app for detecting scenes with text from video input
Apache License 2.0
1 stars 0 forks source link

Releasing 2.0 #45

Closed marcverhagen closed 10 months ago

marcverhagen commented 11 months ago

Overview

This version accepts runtime parameters and includes positional information.

Additions

Changes

keighrim commented 11 months ago

I think https://github.com/clamsproject/app-swt-detection/issues/24 should also be resolved in this release.

marcverhagen commented 11 months ago

Okay, that won't hurt, but I may not be able to get to that till Tuesday or Wednesday.

keighrim commented 11 months ago

Looks like there's a bug in classifier.py

$ podman  run -d --rm --name clams-app-swt-detection -p 5501:5000 -v /mnt:/mnt clams-app-swt-detection:v2  

$ clams source  video:/mnt/llc/llc_data/clams/wgbh/NewsHour/cpb-aacip-507-7659c6sk9k.mp4  | curl -d@- localhost:5501
{
  "metadata": {
    "mmif": "http://mmif.clams.ai/1.0.0"
  },
  "documents": [
    {
      "@type": "http://mmif.clams.ai/vocabulary/VideoDocument/v1",
      "properties": {
        "mime": "video",
        "id": "d1",
        "location": "file:///mnt/llc/llc_data/clams/wgbh/NewsHour/cpb-aacip-507-7659c6sk9k.mp4"
      }
    }
  ],
  "views": [
    {
      "id": "v_0",
      "metadata": {
        "timestamp": "2023-12-22T22:11:40.925876",
        "app": "http://apps.clams.ai/swt-detection",
        "error": {
          "message": "<class 'ZeroDivisionError'>: float division by zero",
          "stackTrace": "  File \"/usr/local/lib/python3.8/site-packages/clams/restify/__init__.py\", line 146, in post\n    return self.json_to_response(self.cla.annotate(raw_data, **self.annotate_param_caster.cast(raw_params)))\n\t\n  File \"/usr/local/lib/python3.8/site-packages/clams/app/__init__.py\", line 116, in annotate\n    annotated = self._annotate(mmif, **runtime_params)\n\t\n  File \"/app/app.py\", line 64, in _annotate\n    predictions = self.classifier.process_video(vd.location)\n\t\n  File \"/app/modeling/classify.py\", line 81, in process_video\n    dur = round(fc / fps, 3) * 1000\n"
        }
      },
      "annotations": []
    }
  ]
marcverhagen commented 11 months ago

Looks like there's a bug in classifier.py

Interesting, never ran into that one, will look into it.

marcverhagen commented 11 months ago

Some remarks on and issues with the new container file.

The way the backbone is loaded is much nicer.

There are advantages to having the one line for the pip install in

RUN pip install -r /app/requirements.txt

But there were reasons for doing

RUN pip install --no-cache-dir torch==2.1.0
RUN pip install --no-cache-dir torchvision==0.16.0
RUN pip install --no-cache-dir pyyaml==6.0.1

One was that certain version configurations allowed by requirements.txt gave errors later on for the app, using requirements-app.txt should deal with that. Another reason was that the app requires less installs than the trainer, which is why there were two requirements files, but with the recent refactoring of the code it looks like the app does now still need to install all packages even those that are only needed for the trainer, in terms of space use this seems a small issue though. I would still like to use the app requirements file because of the version issue.

The image has now grown from 8GB to 10GB because the --no-cache-dir option was removed.

Copying all files to the image before doing the pip install means that those installs are done over and over again each time a small change to the code was made and the image was recreated. I will probably use something like

WORKDIR /app
COPY requirements-app.txt .
RUN pip install --no-cache-dir -r /app/requirements-app.txt
COPY . .
RUN python /app/dl_backbone.py
CMD ["python3", "app.py", "--production"]

Finally, the CMD invocation in the container file uses a configuration script that does not exist anymore. There is a correct default config file name, but that should have been overwritten by the way the app was invoked and I am a bit puzzled why that did not throw an error.

marcverhagen commented 11 months ago

For the ZeroDIvisionError problem raised above, see https://github.com/clamsproject/app-swt-detection/issues/49

marcverhagen commented 11 months ago

The one thing I may want to do for this release is to add some error handling to deal with the above (issue https://github.com/clamsproject/app-swt-detection/issues/49). We could leave that for the next version though.