Skip to content

feat: add map function #1187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
anna-charlotte merged 51 commits into from
Mar 3, 2023
Merged

feat: add map function #1187

anna-charlotte merged 51 commits into from
Mar 3, 2023

Conversation

@anna-charlotte
Copy link
Contributor

@anna-charlotte anna-charlotte commented Feb 28, 2023 •
edited
Loading

Goals:

Add map_docs function and map_batch:

docs = list(map_docs(da, lambda x : x, num_worker=4 ))

This will be different from doing

for doc in da:
    doc = f(doc)

  • leverage multiprocessing by benchmarking in test, check that using 2 CPUS is faster then using 1

  • map_docs()

  • map_docs_batch()

  • benchmarking tests

  • check and update documentation, if required. See guide

@anna-charlotte anna-charlotte mentioned this pull request Feb 28, 2023
Closed
47 tasks
@anna-charlotte anna-charlotte linked an issue Feb 28, 2023 that may be closed by this pull request
Map and apply with multiprocessing #1173
Closed
@JohannesMessner
Copy link
Member

JohannesMessner commented Mar 1, 2023

Is apply_batched also in scope for this issue? And map/map_batched?

JohannesMessner
JohannesMessner reviewed Mar 1, 2023
View reviewed changes
docarray/utils/apply.py Outdated
func: Callable[[BaseDocument], BaseDocument],
num_worker: Optional[int] = None,
pool: Optional['Pool'] = None,
show_progress: bool = False,
Copy link
Member

@JohannesMessner JohannesMessner Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a backend options to switch between multi-processing and multi-threading?

Copy link
Contributor Author

@anna-charlotte anna-charlotte Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understood we only wanted to keep multiprocessing, not multithreading @samsja

Copy link
Member

@samsja samsja Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoanFM maybe know more. But to me only multi processing makes sense. Can multi threading really improve performance here ?

Copy link
Member

@JohannesMessner JohannesMessner Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the keep the PR discussion up to date with what we discussed on Discord: there will be multi threading since it makes sense for IO bound ops and tf/np/torch stuff

@anna-charlotte
Copy link
Contributor Author

anna-charlotte commented Mar 1, 2023

Is apply_batched also in scope for this issue? And map/map_batched?

Map is already included, but private right now. Not sure if we want to expose it again, or only one of the two.
The batched one, I can add in this PR, but right now I am still trying to get the benchmark to work correctly. Test is passing in the CI, but only because it is being skipped I think, because there in only 1 CPU available I think, therefore no performance comparison with 1 cpu vs 2 cpus.

samsja
samsja reviewed Mar 2, 2023
View reviewed changes
JohannesMessner
JohannesMessner requested changes Mar 3, 2023
View reviewed changes
Copy link
Member

@JohannesMessner JohannesMessner left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! We will make the map functions public, right?

JohannesMessner
JohannesMessner requested changes Mar 3, 2023
View reviewed changes
@anna-charlotte
Copy link
Contributor Author

anna-charlotte commented Mar 3, 2023

Looks good! We will make the map functions public, right?

I dont really have an opinion on this. @samsja suggested to keep only one of the two exposed, map or apply. Why would you prefer this @samsja

@anna-charlotte anna-charlotte marked this pull request as ready for review March 3, 2023 09:38
samsja
samsja reviewed Mar 3, 2023
View reviewed changes
@JohannesMessner
Copy link
Member

JohannesMessner commented Mar 3, 2023

Looks good! We will make the map functions public, right?

I dont really have an opinion on this. @samsja suggested to keep only one of the two exposed, map or apply. Why would you prefer this @samsja

Why should we keep only one @samsja? I think both make sense, the user might already have an in-place or pure function that they want to use without rewriting

samsja
samsja reviewed Mar 3, 2023
View reviewed changes
anna-charlotte added 17 commits March 3, 2023 14:30
fix: add tests for func da to doc and da to other len da
18a377b 
Signed-off-by: anna-charlotte <[email protected]>
fix: revert last commit
245283f 
Signed-off-by: anna-charlotte <[email protected]>
test: add len assert
76fe8b7 
Signed-off-by: anna-charlotte <[email protected]>
test: add assertions
34b7f9c 
Signed-off-by: anna-charlotte <[email protected]>
test: add test to for da extend in batch apply
c7a968d 
Signed-off-by: anna-charlotte <[email protected]>
test: extend with only one doc
6cf8ed2 
Signed-off-by: anna-charlotte <[email protected]>
test: fix
5dc9e6d 
Signed-off-by: anna-charlotte <[email protected]>
fix: test
d3fc203 
Signed-off-by: anna-charlotte <[email protected]>
fix: test
45cdc4a 
Signed-off-by: anna-charlotte <[email protected]>
fix: set docs in apply
9839602 
Signed-off-by: anna-charlotte <[email protected]>
fix: indices
87a93ff 
Signed-off-by: anna-charlotte <[email protected]>
fix: indices
eeb7fae 
Signed-off-by: anna-charlotte <[email protected]>
fix: indices
72aaf21 
Signed-off-by: anna-charlotte <[email protected]>
fix: indices
c0f8029 
Signed-off-by: anna-charlotte <[email protected]>
fix:test
9b83c1f 
Signed-off-by: anna-charlotte <[email protected]>
fix: mypy
7638d86 
Signed-off-by: anna-charlotte <[email protected]>
fix: type hint
4a3a290 
Signed-off-by: anna-charlotte <[email protected]>
@anna-charlotte anna-charlotte changed the title feat: add apply function feat: add map function Mar 3, 2023
anna-charlotte added 2 commits March 3, 2023 15:15
fix: remove apply, only keep map
38aae7a 
Signed-off-by: anna-charlotte <[email protected]>
refactor: map to map_docs
01900c9 
Signed-off-by: anna-charlotte <[email protected]>
samsja
samsja reviewed Mar 3, 2023
View reviewed changes
samsja
samsja reviewed Mar 3, 2023
View reviewed changes
anna-charlotte added 2 commits March 3, 2023 15:51
fix: apply suggestion
f6921e0 
Signed-off-by: anna-charlotte <[email protected]>
docs: add example usage
c3fb041 
Signed-off-by: anna-charlotte <[email protected]>
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

📝 Docs are deployed on https://ft-feat-map-apply--jina-docs.netlify.app 🎉

@anna-charlotte anna-charlotte merged commit 6cd5912 into feat-rewrite-v2 Mar 3, 2023
@anna-charlotte anna-charlotte deleted the feat-map-apply branch March 3, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JohannesMessner JohannesMessner JohannesMessner approved these changes

@samsja samsja samsja approved these changes

Assignees

No one assigned

Labels

area/cicd area/core area/housekeeping area/setup area/testing component/array size/l

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Map and apply with multiprocessing

4 participants

@anna-charlotte @JohannesMessner @samsja