Any comments?

Let me first say that this is a great initiative. It does, however, highlight some sore points about our current way of assembling, configuring and running experiments. The assembly of a graph is not easily done by if-statements and linear control code. In your case it is even two largely overlapping scripts. Also, the configuration of the options is not easily done by if-statements (how to discover what objects are there, legal combinations etc). The idea of using the objects themselves to populate some of the options is a great one. I am not sure how to deal with the other issues.
Again, I do not think we should gate the addition of these scripts, I just hope we can come up with some solutions to these problems.

configs/learning_gem5/part1/caches.py (Diff revision 1)

This is a neat concept, and something that extends far beyond a simple education script imho.
I am not discouraging the use of this idea, I just think we should not use it in the examples until that is "the way things are done".

Show all issues

1. Jason Lowe-Power Aug. 18, 2015, 8:17 a.m. (Aug. 18, 2015, 8:17 a.m.)

   I totally agree with you that this is beyond the scope of an education script, and that an education script should show "the way things are done." However, I think that something like this is significantly better than the current "way things are done" in the scripts. 
   IMO, the whole configs/ directory needs a total overhaul, but I don't see that happening any time soon. Therefore, I chose to take a random step here in this new from-scratch script to see what others think. 
   Do you have a specific suggestion to replace SimpleOpts? One thing that is very important to me for these scripts is to have precisely one place where defaults are defined. I'm open to suggestions.

configs/learning_gem5/part1/two_level.py (Diff revision 1)

There is a rather unfortunate lack of re-use between the two scripts. I realise that they are intended to be clear and description (not short), but it irks me that they are so verbose.
Could we use something like the base_config in the test.configs?

Show all issues

1. Jason Lowe-Power Aug. 18, 2015, 8:17 a.m. (Aug. 18, 2015, 8:17 a.m.)

   The lack of re-use was purposeful. I meant each of these two scripts to be totally self contained. They kind of build off of one another in the text of the book, but the point I was going for is "once you finish chapter 1, look at simple.py to make sure your script matches" and "once you finish chapter 2, look at two_level.py to make sure your script matches". 
   My purpose here isn't to make code which is useful outside the scope of the learning gem5 book. The goal is to include the learning gem5 scripts, following the book as closely as possible, so that the regression tester will catch any changes that affect the book.

configs/learning_gem5/part1/two_level.py (Diff revision 1)

Odd indentation

Show all issues

1. Jason Lowe-Power Aug. 18, 2015, 8:17 a.m. (Aug. 18, 2015, 8:17 a.m.)

   Good catch.

I'll second Andreas in saying thanks for the initiative to put this together.  I agree that the whole config script environment needs a thorough restructuring for better modularity and flexibility.  I also understand, based on your responses to Andreas, that that's not your goal here, and that comes out when reading your tutorial.  I think the one thing that would be really valuable would be extend the README here to make that goal more explicit, adding in disclaimers etc. about how these scripts are for educational purposes only, not intended to be used outside the scope of the tutorial, etc.---basically encapsulating the caveats from your responses to Andreas to clarify things for people that stumble on these directly and don't come looking for them after reading the tutorial.

configs/common/SimpleOpts.py (Diff revision 2)

Note that optparse is deprecated (https://docs.python.org/2/library/optparse.html) and users are encouraged to switch to argparse.  It's probably not appropriate to make that change here, but I wanted to mention it, because if/when we do make the plunge of revamping the whole framework we should take that into account at that time.

A few little things, fine with me to ship it once these are fixed.

configs/learning_gem5/README (Diff revisions 2 - 3)

i'd quote the title... "Learning gem5"

configs/learning_gem5/README (Diff revisions 2 - 3)

necessarily (sp), quote title again

Some minor questions on the choice of style.

configs/learning_gem5/part1/caches.py (Diff revision 3)

There is an assymmetry here between calling connectCPU with a port, vs connectBus with the actual crossbar.
Perhaps move connectCPU to I and D Cache subclasses and connect to the appropriate port?
I am also not sure about the value of hiding this. It definitley does not save any typing. Also, in quite a few places you still to the porta = portb explicitly. Is there a reason for not simply doing it all explicitly?

Show all issues

1. Jason Lowe-Power Sept. 6, 2015, 1:18 p.m. (Sept. 6, 2015, 1:18 p.m.)

   Good point about the asymmetry. I've fixed it. Interestingly, I think I had it the way you suggested the first time. I'm not sure why I changed my mind.
   As far as hiding it, I agree that in the particular situation it doesn't make much of a difference. However, I'm trying to make a bigger point that encapsulation is good. IMO pushing things like this into classes is what will make the gem5 config scripts tractable. From the "Learning gem5" text: "However, connecting lots of objects up to complex interconnects can make configuration files quickly grow and become unreadable."