feat: add `transform_request_function` for `enqueue_links` #923

Mantisus · 2025-01-20T15:58:33Z

Description

Add transform_request_function for enqueue_links
Add example in introduction section
Add tests

Issues

Closes: Add Support for Specifying Headers in context.enqueue_links Method #894

janbuchar

Very opinion based, but we should consider returning some other special value for skipping a request instead of None. Seeing e.g. return Request.SKIP would be more readable than return None.

Moreover, an empty transform function would mean "skip all requests", which I'm not too fond of - I'd expect that to just let any request go through without any modification.

docs/introduction/code/03_transform_request.py

Mantisus · 2025-01-21T08:28:26Z

Very opinion based, but we should consider returning some other special value for skipping a request instead of None. Seeing e.g. return Request.SKIP would be more readable than return None.

Moreover, an empty transform function would mean "skip all requests", which I'm not too fond of - I'd expect that to just let any request go through without any modification.

I like the use of some special flag, in terms of readability. But I'm not sure how convenient it would be in regular use.

Also the current behavior is closer to TS, if I'm not mistaken about the filter operation

if (transformRequestFunction) {
    requestOptions = requestOptions
        .map((request) => transformRequestFunction(request))
        .filter((r) => !!r) as RequestOptions[];
}

janbuchar · 2025-01-21T09:15:32Z

src/crawlee/_request.py

@@ -108,6 +108,23 @@ def __eq__(self, other: object) -> bool:
 user_data_adapter = TypeAdapter(UserData)


+class RequestOptions(TypedDict, total=False):


Do we need both this and BaseRequestData? If we could pick one and drop the other, I'd be pretty happy. If we could drop both, I'd be super super happy 😁

Yeah, please no more Request-whatever.

I'm sorta in favor of RequestOptions here - it is simpler to handle than Request and the dict structure invites people to mutate it, if you know what I mean. On the other hand, shoving the full Request in would let us use validation on assignment, which may be more user friendly than getting an error only after the options are used to construct a Request.

I preferred RequestOptions because it better matches the required functionality and behavior of the TS version. And it also allows not to think about cases where, for example, unique_key was changed after Request was created....

And yes, a dictionary is a basic data type that all users understand.

janbuchar · 2025-01-21T09:24:36Z

Very opinion based, but we should consider returning some other special value for skipping a request instead of None. Seeing e.g. return Request.SKIP would be more readable than return None.

I like the use of some special flag, in terms of readability. But I'm not sure how convenient it would be in regular use.

I imagine something like this - it would be cleaner to put the special flags in a separate enum and not Request, but you usually import that anyway. Thoughts?

def transform(request_options: RequestOptions) -> RequestOptions | Literal[Request.SKIP, Request.UNCHANGED]:
    if 'page' in request_options["url"]:
        return {*request_options, "label": "page"}
    if 'campaign' in request_options["url"]:
        return Request.SKIP
    return Request.UNCHANGED

Also the current behavior is closer to TS, if I'm not mistaken about the filter operation

if (transformRequestFunction) {
    requestOptions = requestOptions
        .map((request) => transformRequestFunction(request))
        .filter((r) => !!r) as RequestOptions[];
}

Yeah, it does filter out results that convert to false. I'd argue that this behavior is counter-intuitive, even though changing it in the JS version would be breaking in subtle ways. I would be happy to sacrifice parity between versions here, but I'm open to the opinions of others - @Pijukatel @vdusek @B4nan

B4nan · 2025-01-21T09:40:37Z

What about returning false explicitly?

I would rather not force users to import anything to achieve this behaviour.

janbuchar · 2025-01-21T09:50:28Z

What about returning false explicitly?

I think that you can't really tell what false means in this context either, but at least it's not the implicit return value...

I would rather not force users to import anything to achieve this behaviour.

I get that, that's why I suggest to put it on Request which you're pretty likely to import anyways. We could also use string literals, but that's not commonplace in Python.

B4nan · 2025-01-21T10:51:31Z

I get that, that's why I suggest to put it on Request which you're pretty likely to import anyways.

I don't think so, at least not in the JS version.

But we can have both, I agree its more readable with some kind of symbol. Docs could use that with a comment saying false also works.

vdusek · 2025-01-22T10:33:45Z

src/crawlee/_types.py

@@ -353,6 +354,8 @@ def __call__(
                - `BeautifulSoupCrawler` supports CSS selectors.
            label: Label for the newly created `Request` objects, used for request routing.
            user_data: User data to be provided to the newly created `Request` objects.
+            transform_request_function: A function that processes request options before creating a request.


Suggested change

transform_request_function: A function that processes request options before creating a request.

transform_request_callback: A function that processes request options before creating a request.

Isn't callback a better name?

Dunno, callback kinda sounds like something that just performs some random side effect. This function actually directly changes the result of enqueue_links

vdusek · 2025-01-22T10:38:57Z

I have no issue with using string literals in combination with the "new" Literal type. The type checker will handle typos and other problems, and users won't need to import anything more.

def transform(request_options: RequestOptions) -> RequestOptions | Literal['skip', 'unchanged']:
    ...

docs/introduction/code/03_transform_request.py

Pijukatel · 2025-01-22T12:19:35Z

Very opinion based, but we should consider returning some other special value for skipping a request instead of None. Seeing e.g. return Request.SKIP would be more readable than return None.

I like the use of some special flag, in terms of readability. But I'm not sure how convenient it would be in regular use.

I imagine something like this - it would be cleaner to put the special flags in a separate enum and not Request, but you usually import that anyway. Thoughts?
def transform(request_options: RequestOptions) -> RequestOptions | Literal[Request.SKIP, Request.UNCHANGED]:
    if 'page' in request_options["url"]:
        return {*request_options, "label": "page"}
    if 'campaign' in request_options["url"]:
        return Request.SKIP
    return Request.UNCHANGED
Also the current behavior is closer to TS, if I'm not mistaken about the filter operation
if (transformRequestFunction) {
    requestOptions = requestOptions
        .map((request) => transformRequestFunction(request))
        .filter((r) => !!r) as RequestOptions[];
}
Yeah, it does filter out results that convert to false. I'd argue that this behavior is counter-intuitive, even though changing it in the JS version would be breaking in subtle ways. I would be happy to sacrifice parity between versions here, but I'm open to the opinions of others - @Pijukatel @vdusek @B4nan

From point of view of a person that sees this function for the first time and is not aware of the JS implementation I would expect this transform_request function to accept Request object, mutate it in place and return nothing. I think we should be more explicit with the naming of functions. This name implies side effects on request.
What we really do is more close to some name like "get_transformed_request_options" ...

I would suggest to make a choice of either doing side effects and not returning anything or creating copy of object and returning it.

janbuchar · 2025-01-22T12:37:40Z

From point of view of a person that sees this function for the first time and is not aware of the JS implementation I would expect this transform_request function to accept Request object, mutate it in place and return nothing. I think we should be more explicit with the naming of functions. This name implies side effects on request. What we really do is more close to some name like "get_transformed_request_options" ...

That's why I wouldn't accept functions that return None - it forces the caller to pass a function that returns either the modified options or one of the two other possible conclusions. Empty functions won't pass the type check.

B4nan · 2025-01-22T15:08:28Z

What we really do is more close to some name like "get_transformed_request_options" ...

Let me disagree, anything that is called get* doesn't imply it can be used to mutate things. I would personally keep the JS name.

janbuchar · 2025-01-23T13:55:11Z

src/crawlee/_request.py

@@ -108,27 +108,57 @@ def __eq__(self, other: object) -> bool:
 user_data_adapter = TypeAdapter(UserData)


-class BaseRequestData(BaseModel):
-    """Data needed to create a new crawling request."""
+class RequestOptions(TypedDict):


Correct me if I'm wrong, but can't we now use RequestOptions to type the kwargs of Request.from_url method?

We can't, because these variables from RequestOptions are used in the from_url method.

janbuchar · 2025-01-23T13:57:27Z

src/crawlee/crawlers/_abstract_http/_abstract_http_crawler.py

@@ -140,20 +140,31 @@ async def enqueue_links(
            selector: str = 'a',
            label: str | None = None,
            user_data: dict[str, Any] | None = None,
+            transform_request_function: Callable[[RequestOptions], RequestOptions | Literal['skip', 'unchanged']]


We should extract this type into some shared module. Users who want to provide a correct return type annotation for their transform_request_function should not need to copy all this.

vdusek

Nice!

docs/introduction/03_adding_more_urls.mdx

vdusek · 2025-01-23T13:55:58Z

src/crawlee/_request.py

-class BaseRequestData(BaseModel):
-    """Data needed to create a new crawling request."""
+class RequestOptions(TypedDict):
+    """Options that can be used to customize request creation.


Ou, so we are getting rid of the BaseRequestData, ok, that sounds good and makes sense. Thanks.

src/crawlee/_request.py

src/crawlee/_types.py

Co-authored-by: Vlada Dusek <[email protected]>

vdusek

Good job!

vdusek · 2025-01-24T11:00:53Z

src/crawlee/_types.py

@@ -44,6 +44,8 @@

 HttpPayload: TypeAlias = bytes

+RequestTransformAction: TypeAlias = Literal['skip', 'unchanged']


not sure whether this is necessary 🙂

feat: add transform_request_function for enqueue_links

48fd4c2

Mantisus self-assigned this Jan 20, 2025

janbuchar reviewed Jan 20, 2025

View reviewed changes

docs/introduction/code/03_transform_request.py Outdated Show resolved Hide resolved

Mantisus added 3 commits January 20, 2025 16:45

fix

ac27f03

update doc

063f7db

change transform_request_function for works with RequestOptions

d1cbd9b

Mantisus requested a review from vdusek January 21, 2025 08:28

janbuchar reviewed Jan 21, 2025

View reviewed changes

vdusek reviewed Jan 22, 2025

View reviewed changes

Pijukatel reviewed Jan 22, 2025

View reviewed changes

docs/introduction/code/03_transform_request.py Outdated Show resolved Hide resolved

add return with Literal

4fbbb7d

Mantisus requested review from janbuchar, vdusek and Pijukatel January 22, 2025 15:11

Mantisus added 2 commits January 22, 2025 15:17

del total=False for RequestOptions

2b6c886

rresolve

a82edca

janbuchar reviewed Jan 23, 2025

View reviewed changes

vdusek reviewed Jan 23, 2025

View reviewed changes

Mantisus and others added 3 commits January 23, 2025 16:13

Update docs/introduction/03_adding_more_urls.mdx

acebe56

Co-authored-by: Vlada Dusek <[email protected]>

Update src/crawlee/_request.py

67a1e01

Co-authored-by: Vlada Dusek <[email protected]>

Update src/crawlee/_types.py

8f4144d

Co-authored-by: Vlada Dusek <[email protected]>

type update

dd9c364

vdusek approved these changes Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `transform_request_function` for `enqueue_links` #923

feat: add `transform_request_function` for `enqueue_links` #923

Mantisus commented Jan 20, 2025

janbuchar left a comment

Mantisus commented Jan 21, 2025

janbuchar Jan 21, 2025

vdusek Jan 22, 2025

janbuchar Jan 22, 2025

Mantisus Jan 22, 2025 •

edited

Loading

janbuchar commented Jan 21, 2025 •

edited

Loading

B4nan commented Jan 21, 2025 •

edited

Loading

janbuchar commented Jan 21, 2025

B4nan commented Jan 21, 2025

vdusek Jan 22, 2025

janbuchar Jan 22, 2025

vdusek commented Jan 22, 2025

Pijukatel commented Jan 22, 2025

janbuchar commented Jan 22, 2025

B4nan commented Jan 22, 2025

janbuchar Jan 23, 2025

Mantisus Jan 23, 2025

janbuchar Jan 23, 2025

vdusek left a comment

vdusek Jan 23, 2025

vdusek left a comment

vdusek Jan 24, 2025

		@@ -108,6 +108,23 @@ def __eq__(self, other: object) -> bool:
		user_data_adapter = TypeAdapter(UserData)


		class RequestOptions(TypedDict, total=False):

	transform_request_function: A function that processes request options before creating a request.
	transform_request_callback: A function that processes request options before creating a request.

		@@ -44,6 +44,8 @@

		HttpPayload: TypeAlias = bytes

		RequestTransformAction: TypeAlias = Literal['skip', 'unchanged']

feat: add transform_request_function for enqueue_links #923

Are you sure you want to change the base?

feat: add transform_request_function for enqueue_links #923

Conversation

Mantisus commented Jan 20, 2025

Description

Issues

janbuchar left a comment

Choose a reason for hiding this comment

Mantisus commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mantisus Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

janbuchar commented Jan 21, 2025 • edited Loading

B4nan commented Jan 21, 2025 • edited Loading

janbuchar commented Jan 21, 2025

B4nan commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdusek commented Jan 22, 2025

Pijukatel commented Jan 22, 2025

janbuchar commented Jan 22, 2025

B4nan commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdusek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdusek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: add `transform_request_function` for `enqueue_links` #923

feat: add `transform_request_function` for `enqueue_links` #923

Mantisus Jan 22, 2025 •

edited

Loading

janbuchar commented Jan 21, 2025 •

edited

Loading

B4nan commented Jan 21, 2025 •

edited

Loading