Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
44 translate cropno crop inference udf to gfmap #48
44 translate cropno crop inference udf to gfmap #48
Changes from 37 commits
de4540d
c749633
1020ee4
bc9bd1a
2ec4ebd
f17e0a8
6ae5da2
4a3b74b
f3f4b15
e0c1d05
433f001
af151f7
44f9651
e0ca616
7968ba0
a579be7
42218f0
919391c
9f105e6
b74ecad
29b3034
3e03ab4
7915b93
005841a
f7d09b9
63722e5
14ff604
5ed426b
b443e8b
2add215
3251919
7b7ca4d
3faef72
aa423c6
8723fae
34e4621
df87509
20746f4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this still needed?
onnxruntime
is included in theworldcereal_deps.zip
. And if it's still needed, shouldn't we also put it on s3?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's much lighter than all the presto dependency so it's a waste of resources to install everything just for
onnxruntime
. Also it's a requirement in GFMAP for the ModelInference subclasses, until the pip dependency feature is more stable.Indeed, could you upload it to the S3 @HansVRP ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that for each single UDF in the chain those dependencies get downloaded/unpacked again? That's also lots of overhead I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case it's just onnxruntime that get's reinstalled (quite light), compared to the full UDF I don't think there is a large difference in execution time, even less with large spatial extents probably
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If cached, they should not be reloaded each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's very high. Need to flag this for further profiling. But for now we can leave it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I have seen it is a potential overkill, 200 km square ran on half the amount of memory for me. @GriffinBabe do you have a job id for a job which needed more memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in fact a bit an overhead, if later on in
presto-worldcereal
we again remap: https://github.com/WorldCereal/presto-worldcereal/blob/main/presto/inference.py#L41So we should consider to immediately use the presto naming here and remove the remapping in
presto-worldcereal