Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

Move jobpy to Docker images #3741

Merged
merged 13 commits into from
Jan 15, 2019
Merged

Move jobpy to Docker images #3741

merged 13 commits into from
Jan 15, 2019

Conversation

Krigpl
Copy link
Contributor

@Krigpl Krigpl commented Jan 11, 2019

Stop supporting src_code as it was before.
The path to the script is passed through extra_data.

Blender and image_metrics images still require an update on Docker hub (will push them after initial approval).

@ghost ghost assigned Krigpl Jan 11, 2019
@ghost ghost added the in progress label Jan 11, 2019
Copy link
Contributor

@maaktweluit maaktweluit left a comment

Choose a reason for hiding this comment

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

LGTM! Nice cleanup!

Will approve after the 2 tests are fixed ( or an issue is added to the # fixme )

@Krigpl
Copy link
Contributor Author

Krigpl commented Jan 11, 2019

@maaktweluit Right, these tests are also failing for me on develop so I just removed "fixme" and see what happens. I might have broken environment :)

@codecov
Copy link

codecov bot commented Jan 15, 2019

Codecov Report

Merging #3741 into develop will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           develop   #3741      +/-   ##
==========================================
+ Coverage     87.6%   87.6%   +<.01%     
==========================================
  Files          214     214              
  Lines        18971   18892      -79     
==========================================
- Hits         16619   16551      -68     
+ Misses        2352    2341      -11

@Krigpl Krigpl merged commit d887b5b into develop Jan 15, 2019
@ghost ghost removed the in progress label Jan 15, 2019
@Krigpl Krigpl deleted the move_jobpy branch January 15, 2019 14:07
@mfranciszkiewicz
Copy link
Contributor

@Krigpl Why has this already been merged?

@Krigpl
Copy link
Contributor Author

Krigpl commented Jan 16, 2019

Why not?

@tworec
Copy link
Contributor

tworec commented Jan 16, 2019

https://hub.docker.com/r/golemfactory/blender/dockerfile

MAINTAINER Artur Zawłocki <artur.zawlocki@imapp.pl>

@@ -190,7 +188,7 @@ def _run_docker_job(self) -> Optional[int]:

params = dict(
image=self.image,
script_src=self.src_code,
script_filepath=self.extra_data['script_filepath'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Why script_filepath is part of extra_data ?
As far as I know, in extra_data are all parameters needed inside docker container. script_filepath is used only as command to run docker, so we don't need to pass it to params.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is a slight abuse of extra_data, but there were no better alternatives at the time. Ideally src_code from ctd would be replaced with script_filepath (or even better with some environment config dict, e.g. to include argv as well) but this requires changes to Golem-Messages so this will be done separately.

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

Successfully merging this pull request may close these issues.

5 participants