Skip to content
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

[WIP] Output for jobs #238

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Skarlso
Copy link
Member

@Skarlso Skarlso commented Mar 7, 2020

@michelvocks Hi! So this is ALMOST working. :D The output is saved, but I'm failing to figure out why the Job which is supposed to come next isn't seeing the depending on job's output.

I tried to update the Argument value in Execute and executeJob or something like that, but the depending job has actually an empty output there. Even though it's saved correctly now in PipelineRun. I'm stuck. :)

This is now done. :)

Related PRs for this change:
Protobuf: gaia-pipeline/protobuf#2
SDK: gaia-pipeline/gosdk#5
And a sample go project to build: https://github.com/Skarlso/go-example/tree/output_for_jobs

This is obviously only done for Go projects. I have not worked on any other of the SDKs...

@Skarlso Skarlso requested a review from michelvocks March 7, 2020 21:45
@Skarlso Skarlso changed the title Output for jobs [WIP] Output for jobs Mar 7, 2020
@codecov-io
Copy link

codecov-io commented Mar 7, 2020

Codecov Report

Merging #238 into master will decrease coverage by 0.4%.
The diff coverage is 30.18%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #238      +/-   ##
=========================================
- Coverage      61%   60.6%   -0.41%     
=========================================
  Files          49      49              
  Lines        4265    4315      +50     
=========================================
+ Hits         2602    2615      +13     
- Misses       1220    1250      +30     
- Partials      443     450       +7
Impacted Files Coverage Δ
plugin/grpc.go 0% <ø> (ø) ⬆️
workers/server/worker.go 44.85% <16.66%> (-0.54%) ⬇️
workers/agent/agent.go 50.11% <16.66%> (-0.49%) ⬇️
plugin/plugin.go 64% <18.75%> (-5.41%) ⬇️
workers/scheduler/scheduler.go 59.04% <44%> (-1.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed1ca01...c07f0b7. Read the comment docs.

@Skarlso
Copy link
Member Author

Skarlso commented Mar 7, 2020

@michelvocks Okay, so I found that the job isn't up to date with its dependencies that's why I can't find the output. But if I pass in the run to execute I can see the updated job outputs. Now, I'm not sure if this is right, please review and say if this isn't how its supposed to work. :)

Code is here: https://github.com/gaia-pipeline/gaia/pull/238/files#diff-f60d96c76e053253962207d834ff9fa3R477

@Skarlso Skarlso linked an issue Mar 7, 2020 that may be closed by this pull request
@Skarlso
Copy link
Member Author

Skarlso commented Mar 10, 2020

Hi @prologic.

So, the testing plan would be to pull this PR and build Gaia. With the respective binaries, you would want to test the output with something like this: https://github.com/Skarlso/go-example/tree/output_for_jobs

The important bit to note here is the fact that outputs only work with depending jobs. Also, if multiple outputs are provided by multiple depending jobs with the same key, only the first found will be returned. The rest is ignored. So keys need to be unique to work but I think that's fine.

You would want to create a pipeline and just run it. Test it with docker and test it with remote workers as well please, I will do the same. :)

And thank you! Do you need more information? Usually how I run Gaia when I'm testing is just clone, and run make. Then go to the frontend and run npm run serve. If you want to test the worker on a remote machine you would run something like this:

https://docs.gaia-pipeline.io/operations/worker/installation/

Please don't hesitate to ask if you got stuck or don't know something. I'll gladly help.

Cheers!

@prologic
Copy link

Sorry haven't had a chance to test this out yet been rather busy with my real day job :) I will try to find some time to look at this soon!

@Skarlso
Copy link
Member Author

Skarlso commented Apr 1, 2020

Thanks, no problem. I know that these are difficult times right now. :)

@Skarlso
Copy link
Member Author

Skarlso commented Apr 15, 2020

I did some more testing on this and appears to be working with workers and remote things as well. Now just needs a proper review if the code change is acceptable by you @michelvocks :).

@prologic Did you manage to do anything with it by any chance? It's okay if not, I'm just asking. :)

Cheers.

@prologic
Copy link

@prologic Did you manage to do anything with it by any chance? It's okay if not, I'm just asking. :)

Sadly no; Been so busy lately :/ Sorry!

@Skarlso
Copy link
Member Author

Skarlso commented Apr 24, 2020

No problem my man. I'll do some testing later on. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Job outputs?
3 participants