move import_thread to a separate file #2349

raulchen · 2018-07-05T06:01:16Z

Move import thread and related functions to a separate file, and make it a class.
Note: this PR is simply a sanitizing refactor, it doesn't change any code logic. Next RP will change the import thread to use its own redis client and local scheduler client to address multi-threading issue. Previous discussion can be found at #2248.

robertnishihara · 2018-07-05T06:33:08Z

python/ray/import_thread.py

+from __future__ import print_function
+
+import pickle
+import ray


import ray should be grouped with the other ray imports below.

The standard style of my previous company is putting all imports before all from ... imports. I checked pep8 and google style, they doesn't seem to have a particular rule regarding this, but this style looks cleaner to me. Do we want to consider this? (That being said, I'll follow the current style in this PR.)

Also, I'm seeing something like import ray.ray_constants as ray_constants, I think we should avoid this and use from ray import ray_constants

robertnishihara · 2018-07-05T06:34:15Z

python/ray/import_thread.py

+
+
+class ImportThread(object):
+    """


The docstring should contain a one-line description, e.g.,

"""Brief one-line description of the class. More details.... Attributes: worker: ... """

robertnishihara · 2018-07-05T06:36:39Z

python/ray/import_thread.py

+        function_id = ray.ObjectID(function_id_str)
+        function_name = function_name.decode("ascii")
+        max_calls = int(max_calls)
+        module = module.decode("ascii")


If you rebase on the current master, all of the decode("ascii") lines should be replaced by ray.utils.decode. E.g., see eaa1110.

This has better behavior in Python 2.

robertnishihara · 2018-07-05T06:37:27Z

python/ray/import_thread.py

+        t.start()
+
+    def _run(self):
+        from ray.worker import log_span, WORKER_MODE


Some of this code was changed in #2306

robertnishihara · 2018-07-05T06:38:43Z

python/ray/import_thread.py

+from __future__ import division
+from __future__ import print_function
+
+import pickle


This should not be pickle, it should be import ray.cloudpickle as pickle (grouped with the ray imports below)

raulchen · 2018-07-05T07:10:09Z

(Copying my previous comment here because it was out-dated)

The standard style of my previous company is putting all imports before all from ... imports. I checked pep8 and google style, they doesn't seem to have a particular rule regarding this, but this style looks cleaner to me. Do we want to consider this? (That being said, I'll follow the current style in this PR.)

Also, I'm seeing something like import ray.ray_constants as ray_constants, I think we should avoid this and use from ray import ray_constants instead.

AmplabJenkins · 2018-07-05T07:16:27Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/6482/
Test FAILed.

robertnishihara · 2018-07-05T07:47:04Z

@raulchen We should defer to whatever the google style guide says (or any Python standards).

@concretevitamin are you more familiar with this?

As for from ray import ray_constants, my personal preference is just import ray.ray_constants, which is more verbose. I don't feel very strongly about this one.

Let's avoid usage of from X import Y as much as possible. I've run into issues where sometimes Y has not been defined yet, so it can be safer to do just import X. I remember running into this in #1774.

AmplabJenkins · 2018-07-05T08:11:38Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/6485/
Test PASSed.

raulchen · 2018-07-05T12:03:38Z

hi @robertnishihara,
Google style organizes imports in 4 groups.
Within each group, imports are sorted lexicographically.
See https://github.com/google/styleguide/blob/gh-pages/pyguide.md#313-imports-formatting.

And it uses import x to import single-level modules, and uses from x.y import z to import multi-level modules.
See https://github.com/google/styleguide/blob/gh-pages/pyguide.md#224-decision

I'll change the code according to the above rules.

Also, import x.y and from x import y should be equivalent in terms of how a module is imported by the interpreter. It only make differences in how you reference y. See https://stackoverflow.com/questions/710551/use-import-module-or-from-module-import
Thus regarding the issue you mentioned above, I don't think from import is the culprit. I didn't dig deep, it seems to me that the problem is circular imports.

AmplabJenkins · 2018-07-05T13:18:57Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/6487/
Test FAILed.

raulchen · 2018-07-06T06:23:31Z

hi @robertnishihara, could you please help re-run the Travis CI? test failures look unrelated. Thank you!

raulchen · 2018-07-06T07:27:39Z

Close and reopen this PR to trigger Travis CI.

robertnishihara · 2018-07-08T20:38:21Z

jenkins, retest this please

robertnishihara · 2018-07-08T20:43:02Z

python/ray/import_thread.py

+                        if self.mode != WORKER_MODE:
+                            if key.startswith(b"FunctionsToRun"):
+                                with profile(
+                                        "ray:import_function_to_run",


I think the test failure in the "xray" Travis job is caused by the name changes in these profile statements. This PR changes them accidentally (due to the rebase). Do you see what I mean?

I see. thank you for pointing out.

AmplabJenkins · 2018-07-08T21:48:48Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/6568/
Test PASSed.

AmplabJenkins · 2018-07-08T21:55:31Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/6569/
Test PASSed.

into import_thread

AmplabJenkins · 2018-07-09T04:05:14Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/6573/
Test PASSed.

AmplabJenkins · 2018-07-09T04:17:58Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/6574/
Test PASSed.

raulchen · 2018-07-09T11:26:11Z

@robertnishihara this PR should be ready now.

robertnishihara reviewed Jul 5, 2018

View reviewed changes

raulchen added 3 commits July 5, 2018 15:05

move import_thread to a separate file

9504d18

sort imports

cbee513

group imports regardless of from

b15633d

raulchen force-pushed the import_thread branch from 4d7fff2 to b15633d Compare July 5, 2018 07:07

re-organize imoprts based on google style

ab1c96a

raulchen closed this Jul 6, 2018

raulchen reopened this Jul 6, 2018

robertnishihara reviewed Jul 8, 2018

View reviewed changes

Update import_thread.py

191d172

raulchen added 3 commits July 9, 2018 10:48

fix event_type names in profile statement

a4a7cec

Merge branch 'import_thread' of https://github.com/ant-tech-alliance/ray

052700f

into import_thread

unify duplicate code

d34e1d2

raulchen mentioned this pull request Jul 10, 2018

Make Ray API thread safe #2385

Closed

3 tasks

pcmoritz approved these changes Jul 13, 2018

View reviewed changes

pcmoritz merged commit d6af507 into ray-project:master Jul 13, 2018

move import_thread to a separate file #2349

move import_thread to a separate file #2349

Uh oh!

Conversation

raulchen commented Jul 5, 2018

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!

raulchen commented Jul 5, 2018

Uh oh!

AmplabJenkins commented Jul 5, 2018

Uh oh!

robertnishihara commented Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AmplabJenkins commented Jul 5, 2018

Uh oh!

raulchen commented Jul 5, 2018

Uh oh!

AmplabJenkins commented Jul 5, 2018

Uh oh!

raulchen commented Jul 6, 2018

Uh oh!

raulchen commented Jul 6, 2018

Uh oh!

robertnishihara commented Jul 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Jul 8, 2018

Uh oh!

AmplabJenkins commented Jul 8, 2018

Uh oh!

AmplabJenkins commented Jul 9, 2018

Uh oh!

AmplabJenkins commented Jul 9, 2018

Uh oh!

raulchen commented Jul 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robertnishihara commented Jul 5, 2018 •

edited

Loading