-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add decoding and encoding to string for Graph #5
base: master
Are you sure you want to change the base?
Conversation
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.
@tehPlayer thanks for the contribution! the overall idea looks good, it's something I postponed because I didn't need it on a first version, glad to have help.
I left some comments on things I should be addresses
Also, could you add a new integration test using these new flows? It's the only way to make sure the encode/decode process is actually working
GraphTest
only checks the arguments generated, but doesn't actually do any processing
lib/imageflow/graph_runner.ex
Outdated
:ok <- Native.destroy(job) do | ||
:ok | ||
{:ok, results} <- print_results(job, graph.outputs) do | ||
if results == [], do: :ok, else: {:ok, results} |
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.
I'm fine with a breaking change here, if a new result type needs to be used.
Returning different types based on the result leads to an inconsistent API.
I'd rather return {:ok, []}
, or even {:ok, %Result{}}
(with a result object that we could later mutate further if needed)`.
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.
Another option would be to just add a separate Graph.get_results(job)
function to retrieve these values afterwards.
Might be worth checking how the .NET wrapper does it, since this one was mostly based on it
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.
I actually liked the get_results
idea, but I still had to change return structure.
I've added required fixes and added an integration test. I've also changed the workflow action, because |
|
||
def run(%Graph{} = graph) do | ||
with {:ok, job} <- Native.create(), | ||
:ok <- add_inputs(job, graph.inputs), | ||
:ok <- add_outputs(job, graph.outputs), | ||
:ok <- send_task(job, graph), | ||
:ok <- save_outputs(job, graph.outputs), | ||
:ok <- Native.destroy(job) do |
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.
It seems this step got left out entirely.
I'm not sure where to fit it now, since I'm assuming we need to do this only after get_results
, but it doesn't seem correct to never de-allocate the job
end | ||
end | ||
|
||
def get_results(job, %Graph{outputs: outputs} = graph) do |
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.
I know I'm the one who suggested this function in the first place, but the API doesn't look correct. As per Elixir standards, Graph
should at least be the first argument.
But even with that, the fact that the caller needs to keep track of two separate values (the original graph
, and the new job
) makes this a clunky API
Also, coupling this with my other comment about Native.destroy
being missing, wouldn't it be better to just make this a private function, and call it automatically in the original run flow?
|
||
{:file, path} -> | ||
{:cont, {:ok, path}} | ||
end |
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.
I might be missreading this, but isnt' this resulting in an :output
that corresponds just to whatever the last output from the job was?
I'm guessing it should instead build a map of all the existing outputs
I wanted to add on-fly conversion, without saving files anywhere on disk, but send the stream directly to GCloud. As I know it is possible using the low-level API, but it was missing from high-level API.
This PR shouldn't affect current I/O, it just adds new features.