CV2-5892 gut video #477
CV2-5892 gut video #477
Conversation
There was a problem hiding this comment.
Awesome that the video code removal is relatively easy!
I'm not very familiar with testing for Alegre yet. Since we are relying on the integration tests to validate that code still works, can you link me to the video integration tests so I can double check what we are testing?
With since we are no longer running video models, are there more dependencies we can tear down?
- Can we also now remove the ffmpeg and avcodec, etc install steps? from the Dockerfiles, etc?
Line 6 in 4e0503d
- Can we remove the entire "threat exchange" codebase and associated build stuff? threatexchange/tmk/cpp/bin/tmk-hash-video.cpp
| return AudioModel(app.config['AUDIO_MODEL']) | ||
|
|
||
| def video_model(): | ||
| return VideoModel(app.config['VIDEO_MODEL']) |
There was a problem hiding this comment.
If we are not reading VIDEO_MODEL config anymore, can we remove it from config?
Line 32 in 4e0503d
There was a problem hiding this comment.
No, we can safely remove I think.
|
Great work. It looks like the integration tests triggered from Check Web are passing as well? @skyemeedan , we cannot remove swig or cmake, which is needed to compile the The threatexchange codebase is used for PDQ image hashing; so, I suggest we leave that until / unless we have already removed image hashing @DGaffney ? It shouldn't be needed for video as the Needless to say, we'll want to make sure we test this extensively locally and on QA. |
Correct that we cannot remove swig or cmake, and I suppose tmkpy may not rely directly on FFMPEG et al, but I'm also not sure. I'll remove it and run a test to see what happens? Same for threatexchange, but that can happen in a separate ticket. |
|
Gutted the dependencies locally and confirmed that |
There was a problem hiding this comment.
Awesome that we are able to remove the libraries!
- looks like the threatexchange removal will be handled on the reopened image ticket? #478
- were you able to rebuild and run tests locally?
Confirmed that when running the tests locally, none of the video tests appeared to fail. That, paired with the tests passing here, paired with specifically checking the |
* CV2-5890 remove any ties to shared model for audio model (#476) * remove audio model config * CV2-5892 gut video (#477) * remove more dependencies * remove another mention of config var * CV2-5891 remove threatexchange (#478) * remove ruby * CV2-5778-change-language-detection-agreement (#474) * Update langid.py to reflect `Both CLD3 and FastText must agree and BOTH of them is more than "Threshold" confident` lowering threshold to 0.7 * update langid.py to check if `fasttext_result['result']['language']` and `cld_result['result']['language']` are not None before calculating the minimum confidence * update langid.py write the if condition in one line * update langid.py to check if fasttext_result['result']['confidence'] and cld_result['result']['confidence'] not None --------- Co-authored-by: Devin Gaffney <itsme@devingaffney.com> Co-authored-by: ahmednasserswe <ahmed.nasser.swe@gmail.com>
3 participants
Description
Removes any final assumptions about video with shared model linkages - one question is wtf was
VideoMatcher? Its in the code but was not actually defined? I'm removing that as well...Reference: CV2-5892
How has this been tested?
Confirmed that when running the tests locally, none of the video tests appeared to fail. That, paired with the tests passing here, paired with specifically checking the tmkpy fingerprint from inside the repl, lead me to conclude with high confidence that this will work fine.
Have you considered secure coding practices when writing this code?
None