Introduces support to launch MLflow Projects on YARN. by vguerra · Pull Request #40 · criteo-forks/mlflow

vguerra · 2019-11-15T09:56:04Z

What changes are proposed in this pull request?

(Please fill in changes proposed in this fix)

How is this patch tested?

(Details)

Release Notes

Is this a user-facing change?

No. You can skip the rest of this section.
Yes. Give a description of this change to be included in the release notes for MLflow users.

(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)

What component(s) does this PR affect?

How should the PR be classified in the release notes? Choose one:

rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
rn/feature - A new user-facing feature worth mentioning in the release notes
rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
rn/documentation - A user-facing documentation change worth mentioning in the release notes

codecov-io · 2019-11-15T10:55:16Z

Codecov Report

Merging #40 into criteo-master will decrease coverage by 0.19%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           criteo-master      #40     +/-   ##
================================================
- Coverage          83.66%   83.46%   -0.2%     
================================================
  Files                 20       20             
  Lines               1010     1010             
================================================
- Hits                 845      843      -2     
- Misses               165      167      +2

Impacted Files	Coverage Δ
R/tracking-server.R	`96.22% <0%> (-3.78%)`	⬇️

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 2960d56...1fd7881. Read the comment docs.

t-henri · 2019-12-04T10:15:51Z

-    conda_path = yaml_obj.get("conda_env")
-    if conda_path and docker_env:
-        raise ExecutionException("Project cannot contain both a docker and "
-                                 "conda environment.")


I am not sure you should remove this part. I think the #CRITEO part below is what you intend to keep only in our fork ? So when you will do the PR upstream you should leave this part so the check that there is no conda + docker env is still done.

No, the purpose of the #CRITEO comments is to provide guidance to people when doing merges from the mlflow repo. When this PR is submitted in the official mlflow repo, this comments should disappear.

t-henri · 2019-12-04T10:16:12Z

-                                 "conda environment.")
+    conda_env = yaml_obj.get("conda_env")
+
+    # CRITEO


Why do you want to keep the pex env support only for Criteo ?

+2
Conda could work also with yarn. What about to split the review and do a specific one to add pex support in mlflow ?

t-henri · 2019-12-04T10:17:28Z

+
+        tracking.MlflowClient().set_tag(active_run.info.run_id,
+                                        yarn.MLFLOW_YARN_APPLICATION_ID,
+                                        submitted_run._skein_app_id)


What is the point of this first tag ? If I understand correctly it is the same as the other one but with mlflow. before so it won't appear in the UI, couldn't we just remove it ?

Yes.. I think this is a left over of the 1st version of the implementation on which I had no clue how the "mlflow.*" tags operated.

will remove

t-henri · 2019-12-04T10:40:56Z

+    yarn_config = backend_config.copy()
+
+    for cfg_key in [YARN_NUM_CORES, YARN_MEMORY, YARN_QUEUE,
+                    YARN_HADOOP_FILESYSTEMS, YARN_HADOOD_CONF_DIR,


Typo here: YARN_HADOOD_CONF_DIR

fixed in c8f11f0

t-henri · 2019-12-04T10:41:21Z

+YARN_MEMORY = 'memory'
+YARN_QUEUE = 'queue'
+YARN_HADOOP_FILESYSTEMS = 'hadoop_filesystems'
+YARN_HADOOD_CONF_DIR = 'hadoop_conf_dir'


Typo: YARN_HADOOD_CONF_DIR

fixed in c8f11f0

t-henri · 2019-12-04T10:41:33Z

+    YARN_MEMORY: "1 GiB",
+    YARN_QUEUE: "defautl",
+    YARN_HADOOP_FILESYSTEMS: '',
+    YARN_HADOOD_CONF_DIR: '',


Typo: YARN_HADOOD_CONF_DIR

fixed in c8f11f0

t-henri · 2019-12-04T10:41:52Z

+            memory=yarn_config[YARN_MEMORY],
+            queue=yarn_config[YARN_QUEUE],
+            hadoop_file_systems=yarn_config[YARN_HADOOP_FILESYSTEMS].split(','),
+            hadoop_conf_dir=yarn_config[YARN_HADOOD_CONF_DIR],


Typo: YARN_HADOOD_CONF_DIR

fixed in c8f11f0

t-henri · 2019-12-04T10:44:38Z

+        if app_state == skein.model.FinalStatus.SUCCEEDED:
+            return RunStatus.FINISHED
+        elif app_state == skein.model.FinalStatus.KILLED:
+            return RunStatus.KILLEDs


extra 's' here

fixed in c8f11f0

t-henri · 2019-12-04T10:48:42Z

I left a few comments, and I also think you should add some tests for Yarn projects like the ones for kubernetes (see tests/projects/test_kubernetes.py)

t-henri · 2019-12-04T11:06:33Z

+
+class YarnSubmittedRun(SubmittedRun):
+    """
+    Documentation goes here


You should add the documentation here

fixed in c8f11f0

vguerra · 2019-12-04T13:02:39Z

thanks for the review @t-henri .. yes.. indeed two things still missing for this:

unit tests for all changes ( project file, SubmitteedRun, etc )
changes in documentation to explain the feature

I think i'll do the testing part for this review and leave the docu changes for a next PR.

As well, project file now can take path to a pex file to be used for runs launched in YARN. Fixing linter issues.

…in MLflowProject file. As well, adding a check for the pex_env file path to make sure it exists.

… files.

Adding testing resources for yarn unit tests.

jdlesage · 2019-12-17T09:56:23Z

+  When an MLflow Project specifies a pex environment, it is shipped on the cluster.
+
+  You can specify a Pex environment for your MLflow project by including a ``pex_env`` entry in your
+  ``MLproject`` file. It correspond to the path on the local filesystem or on hdfs where the pex can be found.


As far I understand, it should support any type of mlflow path.... so if pex is on http is ok too
I would say only path and remove local and hdfs part of the sentence

jdlesage · 2019-12-17T09:57:37Z

+     The queue to submit the application to.
+   - ``hadoop_filesystems``
+      List of Hadoop file systems to acquire delegation tokens for.
+   - ``hadoop_conf_dir``


Could be optional and use HADOOP_CONF_DIR if set ?

jdlesage · 2019-12-17T09:59:41Z

-                                 "conda environment.")
+    conda_env = yaml_obj.get("conda_env")
+
+    # CRITEO


+2
Conda could work also with yarn. What about to split the review and do a specific one to add pex support in mlflow ?

jdlesage · 2019-12-17T10:01:28Z

+    kind_of_script = command_parts.pop(0)
+    if 'python' != kind_of_script:
+        raise ExecutionException("Running on YARN backend supports only python jobs. "
+                                 "You are using '%s'" % (kind_of_script))


AFAIK it is a limitation of pex not yarn. I suggest to remove it (and make separate review for pex and try to fix it differently)

jdlesage · 2019-12-17T10:02:12Z

+    env.update(
+        {
+            'SKEIN_CONFIG': './.skein',
+            'PEX_ROOT': "./.pex",


Same it is pex relative... that's an option to inject in pex mode only

jdlesage · 2019-12-17T10:03:41Z

+    )
+
+
+def _is_hdfs_path(path):


har ?
I am pretty sure the prefix => hdfs are already registred in mlflow elsewhere. Could you reuse it.

(and BTW, you can even imagine to use yarn without hdfs)

jdlesage · 2019-12-17T10:04:21Z

+    python_bin = "./%s" % os.path.basename(pex_env) if pex_env.endswith(
+        '.pex') else "./%s/bin/python" % os.path.basename(pex_env)


Same Remove it now... let's see how to manage pex more globaly

jdlesage · 2019-12-17T10:05:16Z

+    dict_files_to_upload = {os.path.basename(path): os.path.abspath(path)
+                            if not _is_hdfs_path(path) else path for path in additional_files}


pretty sure skein can manage out of the box hdfs path.... filter is useless for me.

codecov-commenter · 2026-04-12T17:43:51Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.46%. Comparing base (cb0a421) to head (1fd7881).
⚠️ Report is 1329 commits behind head on criteo-master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                Coverage Diff                @@
##           criteo-master      #40      +/-   ##
=================================================
- Coverage          83.66%   83.46%   -0.20%     
=================================================
  Files                 20       20              
  Lines               1010     1010              
=================================================
- Hits                 845      843       -2     
- Misses               165      167       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

vguerra force-pushed the yarn-backend branch from a2364a4 to 3472e1b Compare December 2, 2019 10:22

t-henri reviewed Dec 4, 2019

View reviewed changes

vguerra added 17 commits December 6, 2019 10:51

Introduces support to launch MLflow Projects on YARN.

0e6077f

fixing all issues with linter.

7aede38

adding skein as dependency

33ef3df

removing dependency on thx

cbabb96

Taking into account hadoop configuration settings.

062edd6

As well, project file now can take path to a pex file to be used for runs launched in YARN. Fixing linter issues.

addressing linter issues with logging statements

97fb91a

fix typo in logging statement

12062a5

handling case of dangerous default arguments

406df81

better handling of exception: using logging.exception api

adf4287

Fixing assertion message in test for when more than 1 env is present …

2abd124

…in MLflowProject file. As well, adding a check for the pex_env file path to make sure it exists.

fixing linter issues

a765754

avoid catching too general exception when fetching skein app logs

d82639f

fixing typo in variable names + adding docu for YarnSubmittedRun

48e8ab5

avoid redefining buil-in exceptions with those from skein

503e9f8

removing unneeded tags + logging info

2d0a6d5

If entry point is a python script, then ship it as part of additional…

109da17

… files.

python 3 -> python 2

986d55f

vguerra added 4 commits December 6, 2019 10:51

adding first unit tests for mlflow project runs on yarn.

b4edb1c

Adding testing resources for yarn unit tests.

fix linter on tests for yarn

308fdcf

composing string on python2 fashion

f2532da

more python3 -> python2 changes

1bf33ad

vguerra force-pushed the yarn-backend branch from 29b53a5 to 1bf33ad Compare December 6, 2019 09:51

vguerra and others added 2 commits December 11, 2019 17:52

Setting up run environment variables for the yarn containers.

55d72eb

Refactored some code in yarn.py, added some tests and documentation

38fe9e8

vguerra commented Dec 13, 2019

View reviewed changes

Comment thread docs/source/projects.rst Outdated

Comment thread docs/source/projects.rst Outdated

Comment thread docs/source/projects.rst

Comment thread mlflow/projects/yarn.py Outdated

t-henri added 3 commits December 13, 2019 16:24

Fix error in doc. Fix error in _parse_yarn_config and added test for it.

1e94735

Support when pex_env is an hdfs path

eca73df

Update MLflow project documentation with pex environment support

63651ec

vguerra requested review from jcuquemelle and jdlesage December 15, 2019 22:35

Improve documentation on Yarn project

1fd7881

jdlesage suggested changes Dec 17, 2019

View reviewed changes

		python_bin = "./%s" % os.path.basename(pex_env) if pex_env.endswith(
		'.pex') else "./%s/bin/python" % os.path.basename(pex_env)

		dict_files_to_upload = {os.path.basename(path): os.path.abspath(path)
		if not _is_hdfs_path(path) else path for path in additional_files}

		)


		def _is_hdfs_path(path):

Uh oh!

Conversation

vguerra commented Nov 15, 2019

What changes are proposed in this pull request?

How is this patch tested?

Release Notes

Is this a user-facing change?

What component(s) does this PR affect?

How should the PR be classified in the release notes? Choose one:

Uh oh!

codecov-io commented Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

t-henri Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t-henri commented Dec 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vguerra commented Dec 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 12, 2026

Codecov Report

Uh oh!

Reviewers

codecov-io commented Nov 15, 2019 •

edited

Loading

t-henri Dec 4, 2019 •

edited

Loading