[pytx] Implement FileContent class #1680

ZeyadTarekk · 2024-11-06T12:14:31Z

Summary

Added FileContent class that detects the type of the file

Test Plan

added some unit tests to test the class

ZeyadTarekk · 2024-11-06T12:16:04Z

@Dcallies Could you check that
#1680

Dcallies

@Dcallies Could you check that
#1680

I am having trouble parsing your comment, did you post this accidentally before completing your thought?

Toplevel comments:

There is some unexpected functionality in here that I think we should talk about more - see questions inline
All of the other ContentTypes today are powered by static/class methods, since we haven't defined what instantiation means yet. I propose we stick with static/class methods to start with, unless you have a specific proposal that depends on making these methods more objective.

Dcallies · 2024-11-06T13:16:01Z

python-threatexchange/threatexchange/content_type/content_base.py

@@ -21,8 +21,8 @@ def get_name(cls) -> str:

    @classmethod
    def extract_additional_content(
-        cls, content_arg: str
-    ) -> t.List[t.Tuple[t.Type["ContentType"], str]]:
+        cls, content_in_file: Path, available_content: t.Sequence[t.Type["ContentType"]]


blocking question: Walk me through your thoughts about these changes. Why are they on this PR?

Dcallies · 2024-11-06T13:17:19Z

python-threatexchange/threatexchange/content_type/file_content.py

+
+class FileContent(ContentType):
+    """
+    Content representing a file. Determines if a file is a photo or video based on file extension.


suggested:

Suggested change

Content representing a file. Determines if a file is a photo or video based on file extension.

ContentType representing a generic file.

Determines if a file is a photo or video based on file extension.

Dcallies · 2024-11-06T13:17:44Z

python-threatexchange/threatexchange/content_type/file_content.py

+    @classmethod
+    def get_name(cls) -> str:
+        return "File"


nit: I think this the same as a default implementation, and so can be removed

Dcallies · 2024-11-06T13:19:08Z

python-threatexchange/threatexchange/content_type/file_content.py

+    def __init__(self, file_name: str):
+        self.file_name = file_name


blocking q: Currently, I don't any ContentType has an implementation for instantiation (we couldn't agree on what it should mean at the time, and so just saved it for the future). What would be your arguments to make file the first one?

Dcallies · 2024-11-06T13:21:08Z

python-threatexchange/threatexchange/content_type/file_content.py

+    Content representing a file. Determines if a file is a photo or video based on file extension.
+    """
+
+    VALID_PHOTO_EXTENSIONS = {".jpg", ".jpeg", ".png", ".gif"}


For photos, we can align with the image types that Pillow supports. Check out this code:

from PIL import Image exts = Image.registered_extensions()

Dcallies · 2024-11-06T13:22:19Z

python-threatexchange/threatexchange/content_type/file_content.py

+    def get_content_type_from_filename(self) -> t.Type[ContentType]:
+        """
+        Determines content type based on file extension.
+        """
+        if any(self.file_name.endswith(ext) for ext in self.VALID_PHOTO_EXTENSIONS):
+            return PhotoContent
+        elif any(self.file_name.endswith(ext) for ext in self.VALID_VIDEO_EXTENSIONS):
+            return VideoContent
+        else:
+            raise ValueError(f"Unknown content type for file: {self.file_name}")


blocking: Since the caller can't easily know whether the type is supported before calling, suggest making this return None instead of throwing an exception.

Dcallies · 2024-11-06T13:26:14Z

python-threatexchange/threatexchange/tests/test_file_content_type.py

+from threatexchange.content_type.video import VideoContent
+from threatexchange.content_type.file_content import FileContent
+
+class TestFileContentType(unittest.TestCase):


blocking: In this library, we use py.test and not unittest.TestCase. Take look at some of the other tests in this file to see how to write those.

The logic overall looks good though!

I can't find any tests that using py.test
All are using unittest.TestCase
@Dcallies

That's a very definitive statement! Here's one: https://github.com/facebook/ThreatExchange/blob/main/python-threatexchange/threatexchange/cli/tests/cli_smoke_test.py

I will open issues for you to convert all of the old ones to py.test :)

Dcallies · 2024-11-06T13:27:17Z

python-threatexchange/threatexchange/content_type/file_content.py

+    def get_name(cls) -> str:
+        return "File"
+
+    def get_content_type_from_filename(self) -> t.Type[ContentType]:


suggested signature

Suggested change

def get_content_type_from_filename(self) -> t.Type[ContentType]:

@classmethod

def get_content_type_from_filename(cls, file_name: str) -> t.Type[ContentType]:

ZeyadTarekk · 2024-11-08T12:37:04Z

I have added all the requested changes
Could you review them @Dcallies

Dcallies

Please:

Convert the test to py.test as previously requested
Make sure lint and testing is passing (you can run it locally before submitting)

Dcallies · 2024-11-08T13:59:25Z

python-threatexchange/threatexchange/tests/test_file_content_type.py

@@ -0,0 +1,33 @@
+import unittest


blocking: Please convert this to py.test as previously requested.

Additionally, we want the tests for modules to live in the same directory as the code. Please move this file to

threatexchange/content_type/tests/test_file_content_type.py

Dcallies · 2024-11-08T14:02:40Z

python-threatexchange/threatexchange/content_type/file_content.py

+    Determines if a file is a photo or video based on file extension.
+    """
+
+    VALID_PHOTO_EXTENSIONS = {ext.lower() for ext in Image.registered_extensions()}


This is doing work during module import time, which is generally considered an anti-pattern, but I don't think it's worth changing

Could you provide more details?

Could you tell me more details?

Here's the first result for me from google for "doing work during module import time"
https://www.benkuhn.net/importtime/

As mentioned, I don't think it's worth changing.

ZeyadTarekk · 2024-11-08T14:30:00Z

Please:

Convert the test to py.test as previously requested

Make sure lint and testing is passing (you can run it locally before submitting)

How to run the linting and testing locally?

Dcallies · 2024-11-08T14:55:03Z

Please:

Convert the test to py.test as previously requested

Make sure lint and testing is passing (you can run it locally before submitting)

How to run the linting and testing locally?

It's still in the same place in https://github.com/facebook/ThreatExchange/blob/main/python-threatexchange/CONTRIBUTING.md :)

Dcallies

blocking: Please make sure all lint and CI are passing. You can test them locally before submitting.

Dcallies · 2024-11-08T14:56:19Z

python-threatexchange/threatexchange/content_type/tests/test_file_content_type.py

+from threatexchange.content_type.video import VideoContent
+from threatexchange.content_type.file_content import FileContent
+
+@pytest.mark.parametrize("file_name,expected_content_type", [


Dcallies · 2024-11-08T14:56:23Z

python-threatexchange/threatexchange/content_type/file_content.py

+    Determines if a file is a photo or video based on file extension.
+    """
+
+    VALID_PHOTO_EXTENSIONS = {ext.lower() for ext in Image.registered_extensions()}


Here's the first result for me from google for "doing work during module import time"
https://www.benkuhn.net/importtime/

As mentioned, I don't think it's worth changing.

Dcallies · 2024-11-08T14:57:50Z

python-threatexchange/threatexchange/content_type/tests/test_file_content_type.py

+    as either PhotoContent or VideoContent based on file extension.
+    """
+    content_type = FileContent.get_content_type_from_filename(file_name)
+    assert content_type == expected_content_type, f"Failed for {file_name}"


You probably don't need to add a message - parameterize will do that for you.

Dcallies · 2024-11-08T14:58:28Z

python-threatexchange/threatexchange/content_type/tests/test_file_content_type.py

+    content_type = FileContent.get_content_type_from_filename(file_name)
+    assert content_type == expected_content_type, f"Failed for {file_name}"
+
+def test_unknown_file_type():


nit: You can combine this into your previous test by just doing

("file.txt", None)

Dcallies

Remember to run linting locally before submitting! I cannot review this PR anymore today - you can save time by running tests and typing yourself!

ZeyadTarekk · 2024-11-08T17:07:28Z

Remember to run linting locally before submitting! I cannot review this PR anymore today - you can save time by running tests and typing yourself!

@Dcallies
I am getting an error and I can't run the tests locally

Dcallies · 2024-11-08T22:05:45Z

Hey @ZeyadTarekk it looks like you are showing the the result of a pip run, which are not the linting tools (black, mypy, pytest). Read more about how pip works to figure out how to avoid installing vpdq to unblock yourself: https://pip.pypa.io/en/stable/cli/pip_install/

Here are my suggestions:

Can you open an issue for the error you are seeing? Include the paste that includes the stacktrace. @ianwal might also respond to a ping on the dangling ; here https://github.com/facebook/ThreatExchange/blame/main/vpdq/cpp/vpdq/cpp/hashing/ffmpegwrapper.cpp#L134
You can ignore vPDQ or install an older version, and omit the extensions directory when testing, then use the relevant python tools to run the CI.

ZeyadTarekk · 2024-11-08T22:15:20Z

Hey @ZeyadTarekk it looks like you are showing the the result of a pip run, which are not the linting tools (black, mypy, pytest). Read more about how pip works to figure out how to avoid installing vpdq to unblock yourself: https://pip.pypa.io/en/stable/cli/pip_install/

Here are my suggestions:

Can you open an issue for the error you are seeing? Include the paste that includes the stacktrace. @ianwal might also respond to a ping on the dangling ; here https://github.com/facebook/ThreatExchange/blame/main/vpdq/cpp/vpdq/cpp/hashing/ffmpegwrapper.cpp#L134

You can ignore vPDQ or install an older version, and omit the extensions directory when testing, then use the relevant python tools to run the CI.

I have already run the linter and it gives me that there is no problem

And when I try to run the tests it is giving me error in the imports because the packages are not installed using pip install --editable .[dev]

Dcallies · 2024-11-09T00:07:39Z

Hey @ZeyadTarekk , @haianhng31 can teach you about a workaround using test selection with pytest, or you can read the command line --help. See the public documentation at https://docs.pytest.org/en/stable/how-to/usage.html

Implement filecontent class

a661224

ZeyadTarekk requested a review from Dcallies as a code owner November 6, 2024 12:14

facebook-github-bot added the CLA Signed label Nov 6, 2024

Dcallies changed the title ~~Implement filecontent class~~ [pytx] Implement FileContent class Nov 6, 2024

Dcallies requested changes Nov 6, 2024

View reviewed changes

ZeyadTarekk added 3 commits November 8, 2024 14:16

discard extract_additional_content changes

b31647e

Change the valid photo types

8aff837

Change the unit tests

d18a8e9

ZeyadTarekk requested a review from Dcallies November 8, 2024 13:05

Dcallies requested changes Nov 8, 2024

View reviewed changes

use pytest in unit tests

95282f0

ZeyadTarekk requested a review from Dcallies November 8, 2024 14:47

Dcallies requested changes Nov 8, 2024

View reviewed changes

ZeyadTarekk added 2 commits November 8, 2024 17:06

modify the tests

69d9abc

Fix linting problems

bb816aa

ZeyadTarekk requested a review from Dcallies November 8, 2024 15:22

ZeyadTarekk added 2 commits November 8, 2024 17:31

Fix error

a5a1930

fix formatting

2580a48

Dcallies requested changes Nov 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pytx] Implement FileContent class #1680

[pytx] Implement FileContent class #1680

ZeyadTarekk commented Nov 6, 2024

ZeyadTarekk commented Nov 6, 2024

Dcallies left a comment

Dcallies Nov 6, 2024

Dcallies Nov 6, 2024

Dcallies Nov 6, 2024

Dcallies Nov 6, 2024

Dcallies Nov 6, 2024

Dcallies Nov 6, 2024

Dcallies Nov 6, 2024

ZeyadTarekk Nov 8, 2024

Dcallies Nov 8, 2024

Dcallies Nov 6, 2024

ZeyadTarekk commented Nov 8, 2024

Dcallies left a comment

Dcallies Nov 8, 2024 •

edited

Loading

Dcallies Nov 8, 2024

ZeyadTarekk Nov 8, 2024

ZeyadTarekk Nov 8, 2024

Dcallies Nov 8, 2024

ZeyadTarekk commented Nov 8, 2024

Dcallies commented Nov 8, 2024

Dcallies left a comment

Dcallies Nov 8, 2024

Dcallies Nov 8, 2024

Dcallies Nov 8, 2024

Dcallies Nov 8, 2024

Dcallies left a comment •

edited

Loading

ZeyadTarekk commented Nov 8, 2024

Dcallies commented Nov 8, 2024

ZeyadTarekk commented Nov 8, 2024

Dcallies commented Nov 9, 2024

		def __init__(self, file_name: str):
		self.file_name = file_name

	def get_content_type_from_filename(self) -> t.Type[ContentType]:
	@classmethod
	def get_content_type_from_filename(cls, file_name: str) -> t.Type[ContentType]:

[pytx] Implement FileContent class #1680

Are you sure you want to change the base?

[pytx] Implement FileContent class #1680

Conversation

ZeyadTarekk commented Nov 6, 2024

Summary

Test Plan

ZeyadTarekk commented Nov 6, 2024

Dcallies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeyadTarekk commented Nov 8, 2024

Dcallies left a comment

Choose a reason for hiding this comment

Dcallies Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeyadTarekk commented Nov 8, 2024

Dcallies commented Nov 8, 2024

Dcallies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dcallies left a comment • edited Loading

Choose a reason for hiding this comment

ZeyadTarekk commented Nov 8, 2024

Dcallies commented Nov 8, 2024

ZeyadTarekk commented Nov 8, 2024

Dcallies commented Nov 9, 2024

Dcallies Nov 8, 2024 •

edited

Loading

Dcallies left a comment •

edited

Loading