ruby: Fix regressions and make Ruby configs Python packages

Information
Submitter:	Andreas Hansson
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11670:aeab4c3c27ef
---------------------------
ruby: Fix regressions and make Ruby configs Python packages

This patch moves the addition of network options into the Ruby module
to avoid the regressions all having to add it explicitly. Doing this
exposes an issue in our current config system though, namely the fact
that addtoPath is relative to the Python script being executed. Since
both example and regression scripts use the Ruby module we would end
up with two different (relative) paths being added. Instead we take a
first step at turning the config modules into Python packages, simply
by adding a __init__.py in the configs/ruby, configs/topologies and
configs/network subdirectories.

As a result, we can now add the top-level configs directory to the
Python search path, and then use the package names in the various
modules. The example scripts are also updated, and the messy
path-deducing variations in the scripts are unified.

Testing Done

Issue Summary

Description	From	Last Updated	Status
I think I'm missing something, but isn't the network directory and the topologies directory both in configs/? Why are these ...	Jason Lowe-Power	Oct. 7, 2016, 8:16 a.m.	Dropped

configs/ruby/Ruby.py (Diff revision 1)

I think I'm missing something, but isn't the network directory and the topologies directory both in configs/? Why are these addToPath calls different?

Show all issues

Andreas Hansson Oct. 7, 2016, 8:15 a.m. (Oct. 7, 2016, 8:15 a.m.)

Excellent question. I think the topology line is acually both wrong and unnecessary.

I really must confess the current behaviuor of addToPath is a bit of a mystery (what is it really relative to), and it turns out it is relative to the run.py script in the test folder.

Jason Lowe-Power Oct. 7, 2016, 8:17 a.m. (Oct. 7, 2016, 8:17 a.m.)
```
Thanks for the info. I agree about addToPath :).
```

Tony Gutierrez Oct. 7, 2016, 3:08 p.m. (Oct. 7, 2016, 3:08 p.m.)

You're right, according to the comment near the definition of addToPath() it seems things are relative to whichever run script you're using. Unfortunately the relative path of configs is not the same for all run scripts. E.g., using apu_se.py by hand still doesn't work, even with this patch, and I have to manually add the paths back to apu_se.py for things to work.

Andreas Hansson Oct. 7, 2016, 3:14 p.m. (Oct. 7, 2016, 3:14 p.m.)

Any suggestions for how to solve it?

Can this patch go in as is? If not, what should I add to it?

Tony Gutierrez Oct. 7, 2016, 4:39 p.m. (Oct. 7, 2016, 4:39 p.m.)

I would suggest we make addToPath() behave more intuitively by having it use the callers path, as opposed to whichever script initiated the python interpreter - in our case the run scripts. This way the configs can truly use paths relative to their own, which is not only more intuitive, but solves the problem of the configs essentially needing to know the path relative to the run scripts, which as previously pointed out may vary. The following worked for apu_se.py paths:
diff --git a/configs/ruby/Ruby.py b/configs/ruby/Ruby.py

--- a/configs/ruby/Ruby.py

+++ b/configs/ruby/Ruby.py

@@ -46,8 +46,8 @@

 from m5.util import addToPath, fatal
import MemConfig

-addToPath('../configs/topologies')

-addToPath('../configs/network')

+addToPath('../topologies')

+addToPath('../network')
import Network
diff --git a/src/python/m5/util/init.py b/src/python/m5/util/init.py

--- a/src/python/m5/util/init.py

+++ b/src/python/m5/util/init.py

@@ -81,8 +81,10 @@
 # if it's a relative path and we know what directory the current
 # python script is in, make the path relative to that directory.


if not os.path.isabs(path) and sys.path[0]:
path = os.path.join(sys.path[0], path)
frame = sys._current_frames().values()[0]
caller_path = os.path.dirname(frame.f_back.f_globals['file'])
if not os.path.isabs(path) and caller_path:
path = os.path.join(caller_path, path)

     path = os.path.realpath(path)

     # sys.path[0] should always refer to the current script's directory,

     # so place the new dir right after that.

Then you'd need to go for all callers of addToPath() to specify paths relative to their own, and not run.py.

Tony Gutierrez Oct. 7, 2016, 4:42 p.m. (Oct. 7, 2016, 4:42 p.m.)

Sorry, the diff in the above got messed up somehow. Should be:

diff --git a/configs/ruby/Ruby.py b/configs/ruby/Ruby.py
--- a/configs/ruby/Ruby.py
+++ b/configs/ruby/Ruby.py
@@ -46,8 +46,8 @@
 from m5.util import addToPath, fatal
 
 import MemConfig
-addToPath('../configs/topologies')
-addToPath('../configs/network')
+addToPath('../topologies')
+addToPath('../network')
 
 import Network
 
diff --git a/src/python/m5/util/__init__.py b/src/python/m5/util/__init__.py
--- a/src/python/m5/util/__init__.py
+++ b/src/python/m5/util/__init__.py
@@ -81,8 +81,10 @@
 
     # if it's a relative path and we know what directory the current
     # python script is in, make the path relative to that directory.
-    if not os.path.isabs(path) and sys.path[0]:
-        path = os.path.join(sys.path[0], path)
+    frame = sys._current_frames().values()[0]
+    caller_path = os.path.dirname(frame.f_back.f_globals['__file__'])
+    if not os.path.isabs(path) and caller_path:
+        path = os.path.join(caller_path, path)
     path = os.path.realpath(path)
     # sys.path[0] should always refer to the current script's directory,
     # so place the new dir right after that.

Andreas Hansson Oct. 10, 2016, 2:50 p.m. (Oct. 10, 2016, 2:50 p.m.)

This is beyond my Python knowledge. I gave it a quick stab and I cannot work out what the right "addPath" would be for e.g. the learning-gem5 regressions that then call another script.

I am happy to push the patch in its current form (as it solves all the regression issues), but I don't know how to best address the bigger issue.

Tony Gutierrez Oct. 10, 2016, 2:58 p.m. (Oct. 10, 2016, 2:58 p.m.)

Ok, I don't want to hold this up. I'll give it a ship it, and I'll take a stab at fixing the path issues more generally.

Ship It!

Description:

~		Changeset 11668:fd9c744bac8b
	~	Changeset 11668:8816ebf0a103

		ruby: Fix broken regressions

		This patch moves the addition of network options into the Ruby module
		to avoid the regressions all having to add it explicitly. The example
~		scripts are also updated.
	~	scripts are also updated, and the messy path-deducing variations in
	+	the scripts are unified.

Diff:

Revision 2 (+15 -77)

Show changes

	configs/example/apu_se.py
	configs/example/fs.py
	configs/example/garnet_synth_traffic.py
	configs/example/ruby_direct_test.py
	configs/example/ruby_gpu_random_test.py
	configs/example/ruby_mem_test.py
	configs/example/ruby_random_test.py
	configs/example/se.py
	11 more

I'm noticing that this patch doesn't completely fix the regressions on its own. There are still two failures that I'm seeing with it applied (although they may be unrelated, not sure):

cat build/HSAIL_X86/tests/opt/quick/se/04.gpu/x86/linux/gpu-ruby-GPU_RfO/simerr

warn: add_child('dst_node_'): child 'int_node' already has parent
warn: add_child('dst_node_'): child 'int_node' already has parent
warn: add_child('dst_node_'): child 'int_node' already has parent
warn: add_child('dst_node_'): child 'dst_node_' already has parent
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "/home/bpotter/Research/gem5/gem5-public-process/src/python/m5/main.py", line 400, in main
exec filecode in scope
File "/home/bpotter/Research/gem5/gem5-public-process/tests/testing/../run.py", line 184, in <module>
execfile(joinpath(tests_root, 'configs', test_filename + '.py'))
File "/home/bpotter/Research/gem5/gem5-public-process/tests/testing/../configs/gpu-ruby.py", line 282, in <module>
Ruby.create_system(options, None, system)
File "/home/bpotter/Research/gem5/gem5-public-process/configs/ruby/Ruby.py", line 175, in create_system
Network.init_network(options, network, InterfaceClass)
File "/home/bpotter/Research/gem5/gem5-public-process/configs/network/Network.py", line 108, in init_network
network.setup_buffers()
File "/home/bpotter/Research/gem5/gem5-public-process/src/mem/ruby/network/simple/SimpleNetwork.py", line 62, in setup_buffers
if link.dst_node == router:
File "/home/bpotter/Research/gem5/gem5-public-process/src/python/m5/SimObject.py", line 1105, in getattr
raise AttributeError, err_string
AttributeError: object 'SimpleIntLink' has no attribute 'dst_node'
(C++ object is not yet constructed, so wrapped C++ methods are unavailable.)

cat build/HSAIL_X86/tests/opt/quick/se/60.gpu-randomtest/x86/linux/gpu-randomtest-ruby-GPU_RfO/simerr

warn: add_child('dst_node_'): child 'int_node' already has parent
warn: add_child('dst_node_'): child 'int_node' already has parent
warn: add_child('dst_node_'): child 'int_node' already has parent
warn: add_child('dst_node_'): child 'dst_node_' already has parent
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "/home/bpotter/Research/gem5/gem5-public-process/src/python/m5/main.py", line 400, in main
exec filecode in scope
File "/home/bpotter/Research/gem5/gem5-public-process/tests/testing/../run.py", line 184, in <module>
execfile(joinpath(tests_root, 'configs', test_filename + '.py'))
File "/home/bpotter/Research/gem5/gem5-public-process/tests/testing/../configs/gpu-randomtest-ruby.py", line 104, in <module>
Ruby.create_system(options, False, system)
File "/home/bpotter/Research/gem5/gem5-public-process/configs/ruby/Ruby.py", line 175, in create_system
Network.init_network(options, network, InterfaceClass)
File "/home/bpotter/Research/gem5/gem5-public-process/configs/network/Network.py", line 108, in init_network
network.setup_buffers()
File "/home/bpotter/Research/gem5/gem5-public-process/src/mem/ruby/network/simple/SimpleNetwork.py", line 62, in setup_buffers
if link.dst_node == router:
File "/home/bpotter/Research/gem5/gem5-public-process/src/python/m5/SimObject.py", line 1105, in getattr
raise AttributeError, err_string
AttributeError: object 'SimpleIntLink' has no attribute 'dst_node'
(C++ object is not yet constructed, so wrapped C++ methods are unavailable.)

Andreas Hansson Oct. 7, 2016, 3:15 p.m. (Oct. 7, 2016, 3:15 p.m.)

I would think this is "unrelated" in that it has nothing to do with the import of Network.

Ship It!

Andreas Hansson Oct. 12, 2016, 1:56 a.m. (Oct. 12, 2016, 1:56 a.m.)

I've taken another stab at fixing this in a less hackish way. I took a first stab at making the shared config modules Python packages (as they should have been all along). Have a look and let me know what you think.

Summary:

-	ruby: Fix broken regressions
+	ruby: Fix regressions and make Ruby configs Python packages

Description:

~		Changeset 11668:8816ebf0a103
	~	Changeset 11670:48761052b362

~		ruby: Fix broken regressions
	~	ruby: Fix regressions and make Ruby configs Python packages

		This patch moves the addition of network options into the Ruby module
~		to avoid the regressions all having to add it explicitly. The example
~		scripts are also updated, and the messy path-deducing variations in
~		the scripts are unified.
	~	to avoid the regressions all having to add it explicitly. Doing this
	~	exposes an issue in our current config system though, namely the fact
	~	that addtoPath is relative to the Python script being executed. Since
	+	both example and regression scripts use the Ruby module we would end
	+	up with two different (relative) paths being added. Instead we take a
	+	first step at turning the config modules into Python packages, simply
	+	by adding a init.py in the configs/ruby, configs/topologies and
	+	configs/network subdirectories.
	+
	+	As a result, we can now add the top-level configs directory to the
	+	Python search path, and then use the package names in the various
	+	modules. The example scripts are also updated, and the messy
	+	path-deducing variations in the scripts are unified.

Diff:

Revision 3 (+50 -111)

Show changes

	configs/example/apu_se.py
	configs/example/fs.py
	configs/example/garnet_synth_traffic.py
	configs/example/ruby_direct_test.py
	configs/example/ruby_gpu_random_test.py
	configs/example/ruby_mem_test.py
	configs/example/ruby_random_test.py
	configs/example/se.py
	15 more

Ship It!

I like this approach! A couple of comments,though.

I think you need to add an init.py file to each directory (ruby, topologies, network) for this to work. Did you forget to hg add them?
Since we don't have good test coverage, did you eyeball that the rest of the scripts in configs/ruby didn't need to be updated similarly? Looking that them, I think AMD_Base_Constructor.py and GPU_VIPER_Region.py should be updated too. I don't think there are any others, but I could be wrong.

Thanks for fixing this the "right" way. Maybe next time someone can factor out all of the common code across all of these files and make them more "modular".

Andreas Hansson Oct. 12, 2016, 7:24 a.m. (Oct. 12, 2016, 7:24 a.m.)

I did hg add the empty init.py files, but somehow they don't appear in the review. Odd. I'll double check.

I did do my best to update the other scripts based on some grepping, but I could very well have missed a few. Thanks for the pointers.

Description:

~		Changeset 11670:48761052b362
	~	Changeset 11670:aeab4c3c27ef

		ruby: Fix regressions and make Ruby configs Python packages

		This patch moves the addition of network options into the Ruby module
		to avoid the regressions all having to add it explicitly. Doing this
		exposes an issue in our current config system though, namely the fact
		that addtoPath is relative to the Python script being executed. Since
		both example and regression scripts use the Ruby module we would end
		up with two different (relative) paths being added. Instead we take a
		first step at turning the config modules into Python packages, simply
		by adding a init.py in the configs/ruby, configs/topologies and
		configs/network subdirectories.

		As a result, we can now add the top-level configs directory to the
		Python search path, and then use the package names in the various
		modules. The example scripts are also updated, and the messy
		path-deducing variations in the scripts are unified.

Diff:

Revision 4 (+160 -113)

Show changes

	configs/example/apu_se.py
	configs/example/fs.py
	configs/example/garnet_synth_traffic.py
	configs/example/ruby_direct_test.py
	configs/example/ruby_gpu_random_test.py
	configs/example/ruby_mem_test.py
	configs/example/ruby_random_test.py
	configs/example/se.py
	20 more

Thanks again for doing this!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 11668:fd9c744bac8b

Changeset 11668:8816ebf0a103

Diff:

Summary:

Description:

Changeset 11668:8816ebf0a103

Changeset 11670:48761052b362

Diff:

Description:

Changeset 11670:48761052b362

Changeset 11670:aeab4c3c27ef

Diff:

Status: Closed (submitted)