feat: Handle request list user input #326

Pijukatel · 2024-11-18T07:08:45Z

Add helper function to handle request list inputs.

Closes: #310

github-actions

⚠️ Pull Request Tookit has failed!

Pull request is neither linked to an issue or epic nor labeled as adhoc!

tests/unit/actor/test_actor_create_request_list.py

src/apify/_actor.py

src/apify/storages/_known_actor_input_keys.py

pyproject.toml

src/apify/storages/_known_actor_input_keys.py

src/apify/_platform_event_manager.py

src/apify/storages/_known_actor_input_keys.py

src/apify/storages/_actor_inputs.py

Finalize tests. Split to its own file.

Fix typing issues WIP

TODO: Finish test for it. WIP

Add test for checking all genrated request properties.

Add helper class for input keys Add top level Input class for handling actor inputs.

Add few more tests for regex

It had too many assumptions that users might not be interested in. Users should create such Input helper classes based on their specific inputs and their names.

Update TCH001, TCH002, TCH003 uses.

Pijukatel · 2024-11-19T13:18:02Z

Sorry for the force-push. Due to my previous mainly Gerrit-based experience I was expecting this Rebase button in Github to do something more clean in the log...

Pijukatel · 2024-11-19T14:11:36Z

pyproject.toml

@@ -141,6 +141,9 @@ indent-style = "space"
 docstring-quotes = "double"
 inline-quotes = "single"

+[tool.ruff.lint.flake8-type-checking]
+runtime-evaluated-base-classes = ["pydantic.BaseModel", "crawlee.configuration.Configuration"]


Even though crawlee.configuration.Configuration inherits from pydantic_settings.BaseSettings and that one from pydantic.BaseModel, ruff has some issues in following inheritance hierarchy too far. So until that changes, some models will have to be explicitly mentioned even though they inherit from pydantic.BaseModel.
See closed issue where this is described as known limitation to certain degree:
astral-sh/ruff#8725

janbuchar

This is looking pretty good! Just a bunch of nits to iron out...

janbuchar · 2024-11-19T13:49:55Z

src/apify/storages/request_list.py

+url_input_adapter = TypeAdapter(list[Union[_RequestsFromUrlInput, _SimpleUrlInput]])
+
+
+class RequestList(CrawleeRequestList):


Please add a @docs_group decorator here with an appropriate group. Also, can you mention that this class also adds the open method?

Not yet available in version used in sdk. Added it as comment for now.

janbuchar · 2024-11-19T13:52:20Z

src/apify/storages/request_list.py

+class _RequestDetails(BaseModel):
+    method: HttpMethod = 'GET'
+    payload: str = ''
+    headers: dict[str, str] = Field(default_factory=dict)
+    user_data: dict[str, str] = Field(default_factory=dict, alias='userData')
+
+
+class _RequestsFromUrlInput(_RequestDetails):
+    requests_from_url: str = Field(alias='requestsFromUrl')


Nit for consistency - we usually use the headers: Annotated[dict[str, str], Field(default_factory=dict)] = {} way of writing pydantic models - it lets you use the default value the way it was originally intended.

Thanks I didn't know it works like this in Pydantic. I was actually trigger by it so much, I had to test, that it is not mutable default input :-)

I get it. It shouldn't even be necessary to have both default_factory and an actual default, but mypy seems to disagree.

src/apify/storages/request_list.py

janbuchar · 2024-11-19T14:01:16Z

tests/unit/actor/test_request_list.py

+    expected_user_data = UserData()
+    if 'userData' in optional_input:
+        for key, value in optional_input['userData'].items():
+            expected_user_data[key] = value
+    assert request.user_data == expected_user_data


Is the copy of optional_input['userData'] necessary?

It is not exactly copy. It is transformation to UserData and that object does not have any convenient dict based constructor. It would be nice to do just UserData(optional_input['userData'])

janbuchar · 2024-11-19T14:11:57Z

tests/unit/actor/test_request_list.py

+    with mock.patch.object(
+        http_client, 'send_request', return_value=_create_dummy_response(mocked_read_outputs)
+    ) as mocked_send_request:


Instead of this, you could either take advantage of respx or just make a mock implementation of BaseHttpClient (probably with unittest.mock)?

If you want to go with mocking, then please mock BaseHttpClient instead of a concrete implementation. I have a feeling that using respx could result in a more readable code - did you try it?

src/apify/storages/request_list.py

janbuchar · 2024-11-19T15:44:28Z

src/apify/storages/_request_list.py

+        name is name of the returned RequestList
+        request_list_sources_input  can contain list dicts with either url or requestsFromUrl key
+        http_client is client that will be used to send get request to url defined in requestsFromUrl


Use the same docblock style as we do in the rest of the code please.

janbuchar · 2024-11-19T15:46:15Z

src/apify/storages/_request_list.py

+        http_client is client that will be used to send get request to url defined in requestsFromUrl
+
+        Example request_list_sources_input:
+            [


We wrap examples in markdown code blocks - https://github.com/apify/crawlee-python/blob/master/src/crawlee/http_crawler/_http_crawler.py#L30-L52

janbuchar · 2024-11-19T15:46:45Z

src/apify/storages/_request_list.py

+        if not http_client:
+            http_client = HttpxHttpClient()
+
+        ulr_inputs = url_input_adapter.validate_python(request_list_sources_input)  # instance of list[Union[...]]


tpyo, plus the comment was just meant for you, it's not that useful in the code.

janbuchar · 2024-11-19T15:50:45Z

tests/unit/actor/test_request_list.py

+    with mock.patch.object(
+        http_client, 'send_request', return_value=_create_dummy_response(mocked_read_outputs)
+    ) as mocked_send_request:


If you want to go with mocking, then please mock BaseHttpClient instead of a concrete implementation. I have a feeling that using respx could result in a more readable code - did you try it?

github-actions bot assigned Pijukatel Nov 18, 2024

github-actions bot added this to the 103rd sprint - Tooling team milestone Nov 18, 2024

github-actions bot added t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels Nov 18, 2024

github-actions bot reviewed Nov 18, 2024

View reviewed changes

Pijukatel commented Nov 18, 2024

View reviewed changes

tests/unit/actor/test_actor_create_request_list.py Outdated Show resolved Hide resolved

Pijukatel commented Nov 18, 2024

View reviewed changes

tests/unit/actor/test_actor_create_request_list.py Outdated Show resolved Hide resolved

Pijukatel commented Nov 18, 2024

View reviewed changes

src/apify/_actor.py Outdated Show resolved Hide resolved

B4nan mentioned this pull request Nov 18, 2024

Compose Actor class of smaller components #327

Open

Pijukatel changed the title ~~Handle request list user input~~ feat: Handle request list user input Nov 18, 2024

Pijukatel marked this pull request as ready for review November 18, 2024 17:28

Pijukatel requested a review from janbuchar November 18, 2024 17:28

Pijukatel commented Nov 19, 2024

View reviewed changes

src/apify/storages/_known_actor_input_keys.py Outdated Show resolved Hide resolved

vdusek self-requested a review November 19, 2024 09:36

vdusek reviewed Nov 19, 2024

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

src/apify/storages/_known_actor_input_keys.py Outdated Show resolved Hide resolved

janbuchar reviewed Nov 19, 2024

View reviewed changes

src/apify/_platform_event_manager.py Show resolved Hide resolved

janbuchar self-requested a review November 19, 2024 09:50

janbuchar reviewed Nov 19, 2024

View reviewed changes

src/apify/storages/_known_actor_input_keys.py Outdated Show resolved Hide resolved

src/apify/storages/_actor_inputs.py Outdated Show resolved Hide resolved

janbuchar reviewed Nov 19, 2024

View reviewed changes

src/apify/storages/_actor_inputs.py Outdated Show resolved Hide resolved

janbuchar reviewed Nov 19, 2024

View reviewed changes

src/apify/storages/_actor_inputs.py Outdated Show resolved Hide resolved

Pijukatel added 10 commits November 19, 2024 13:59

Draft example of helper function to create RequestList

cc94023

Add test for simple input

ded055d

WIP

57dd329

WIP

0a465be

Finalize tests. Split to its own file.

Use Pydantic to handle raw inputs

4b16737

Fix typing issues WIP

Properly pass request creation settings.

f2c2440

TODO: Finish test for it. WIP

Add tests fro regexp.

5af9405

Add test for checking all genrated request properties.

Use regex instead of re.

cf4534a

Add helper class for input keys Add top level Input class for handling actor inputs.

Use re with \w

ff3e047

Add few more tests for regex

Reduce some test code repetition.

b4ad24f

Pijukatel added 5 commits November 19, 2024 13:59

Remove types-regex

05d048a

Make ruff happy

376ae8b

Remove Input class

629939e

It had too many assumptions that users might not be interested in. Users should create such Input helper classes based on their specific inputs and their names.

Review comments.

910d11f

Update TCH001, TCH002, TCH003 uses.

Remove unnecessary pyproject setting value.

6ff9e90

Pijukatel force-pushed the handle-request-list-user-input branch from c237c66 to 6ff9e90 Compare November 19, 2024 12:59

Pijukatel commented Nov 19, 2024

View reviewed changes

janbuchar requested changes Nov 19, 2024

View reviewed changes

Addresing review comments

3f33145

janbuchar reviewed Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Handle request list user input #326

feat: Handle request list user input #326

Pijukatel commented Nov 18, 2024 •

edited by vdusek

Loading

github-actions bot left a comment

Pijukatel commented Nov 19, 2024

Pijukatel Nov 19, 2024

janbuchar left a comment

janbuchar Nov 19, 2024

Pijukatel Nov 19, 2024

janbuchar Nov 19, 2024

Pijukatel Nov 19, 2024

janbuchar Nov 19, 2024

janbuchar Nov 19, 2024

Pijukatel Nov 19, 2024

janbuchar Nov 19, 2024

Pijukatel Nov 19, 2024

janbuchar Nov 19, 2024

janbuchar Nov 19, 2024

janbuchar Nov 19, 2024

janbuchar Nov 19, 2024

janbuchar Nov 19, 2024

		url_input_adapter = TypeAdapter(list[Union[_RequestsFromUrlInput, _SimpleUrlInput]])


		class RequestList(CrawleeRequestList):

feat: Handle request list user input #326

Are you sure you want to change the base?

feat: Handle request list user input #326

Conversation

Pijukatel commented Nov 18, 2024 • edited by vdusek Loading

github-actions bot left a comment

Choose a reason for hiding this comment

Pijukatel commented Nov 19, 2024

Choose a reason for hiding this comment

janbuchar 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

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

Pijukatel commented Nov 18, 2024 •

edited by vdusek

Loading