add compute getResult #983
Conversation
alexcos20
added
Status: InProgress
|
Please DO NOT merge, needs updated versions of provider/C2D components. |
|
what is |
Maximum amount of time that an algorithm is allowed to run in a C2d env. If service timeout > algoTimeLimit, then algoTimeLimit is enforced |
| this.logger.error(e) | ||
| throw e | ||
| } | ||
| return destination |
There was a problem hiding this comment.
what's the point of returning this? You have destination as a parameter and then it's never reassigned
There was a problem hiding this comment.
yeah, it's overkill. any result !== null means that the download succeeded
| * @param {number} index Compute Result index | ||
| * @param {string} destination: destination folder | ||
| * @param {string} did Decentralized identifier. | ||
| * @param {DDO} ddo If undefined then the ddo will be fetched by did, this is just to optimize network calls |
There was a problem hiding this comment.
so sounds like a param like asset: DDO | DID | string and then check what you get passed would be nicer pattern here to limit parameters
| * @param {string} destination: destination folder | ||
| * @param {string} did Decentralized identifier. | ||
| * @param {DDO} ddo If undefined then the ddo will be fetched by did, this is just to optimize network calls | ||
| * @param {ServiceCompute} service If undefined then we get the service from the ddo |
There was a problem hiding this comment.
where else would we get it from? Why can another compute service be passed to this method which seems to be then different from the DDO compute service? What's the use case for this? Something like "I want the results for a random compute service I just came up with, but for this existing DDO which does not have this compute service"?
There was a problem hiding this comment.
it's for use cases where you already have the service, but not the ddo. This was actually from the market
There was a problem hiding this comment.
If I see this right, we always have full DDOs in this getComputeJobs method:
https://github.com/oceanprotocol/market/blob/main/src/utils/compute.ts#L257
we just don't pass it as part of ComputeJobMetaData but we actually could:
https://github.com/oceanprotocol/market/blob/main/src/utils/compute.ts#L188
Cause then we could remove that passing only a service use case from here. By always requiring passing of an asset which then is either a DDO or just string/DID we have made 3 parameters into 1 too
|
Code Climate has analyzed commit ee75240 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 84.6% (50% is the threshold). This pull request will bring the total coverage in the repository to 81.1% (0.1% change). View more on Code Climate. |
| index: number, | ||
| destination: string, | ||
| account: Account | ||
| ): Promise<any> { |
There was a problem hiding this comment.
based on typings and underlying code this method returns a string
| did?: string, | ||
| ddo?: DDO, | ||
| service?: ServiceCompute | ||
| ): Promise<any> { |
There was a problem hiding this comment.
based on underlying typing and code of provider.computeResult() this returns a Promise<string>
|
So main use case to me would be: "I have a |
|
@kremalicious the flow is the following
The UI will not change. The difference is that we now call |
perfect, thanks for explanation! So that's already good guidance on what needs to be done over in market for this |
|
Already linked this in the issue |
Closes #982
export interface ComputeLimits {
algoTimeLimit?: string
storageExpiry?: string
}
(both expressed in seconds)
this allows the market to expose computeLimits to users