On 02/02/2012 05:30 PM, Andy Doan wrote:
Starting a new thread to deal with this project.
We now have this project hosted at:
http://android.git.linaro.org/gitweb?p=lava-server/android_benchmark_views.git;a=summary
I've made a number of improvements since this was initially sent out including:
Hi Andy.
I'm reading the code now. I have some comments. Essentially before this lands you _have_ to add one change, then we can follow up with iterative improvements.
You _have_ to apply south migrations to your application. Without that we will not be able to make reasonable upgrades. Adding migrations later is not an option as there is a bit that is very hard to automate.
Fortunately this is pretty easy:
1) Add a dependency on south (install_requires in setup.py) 2) Run lava-server manage ... android_benchmark_views_app schemamigration --initial --auto 3) Inspect the code briefly and add it to version control 4) Push that up for review.
As for the rest:
*) helpers.py, don't use double leading underscore. *) Try keeping the code that relates to your model in the model class, also add a few one liners if you can to document each public method *) extension.py, in def version(), pass the second argument to versiontools.format_version() please pass your main package object (android_benchmark_views_app). This will make it follow git version hashes. *) models.py, BenchmarkRun, each ForeignKey should have a related_name. This name is used when accessing the model field from the "remote" end, then you can navigate back using this name. In general this is required in some cases (when there are clashes, anyway this is beyond the scope of this conversation) *) models.py BenchmarkReport, don't use comments like that. Firs of all, in django it's very very bad to use nullable text/char fields. Instead always use null=False, blank=True - that makes writing code then saner. As for comments. Instead of using a simple text field please consider using django comment system. It _is_ slightly more complicated than using a free-for-all text area (like on launchpad's whiteboard but at the same time you get much more in return. You can read about this here: https://docs.djangoproject.com/en/dev/ref/contrib/comments/ *) models.py both classes, add a get_absolute_url() method based on django permalink system. It's trivial to use, see this document: https://docs.djangoproject.com/en/dev/ref/models/instances/#django.db.models... *) You ship query.flot. While that's okay for now I'd rather see this managed by a dedicated django app that just wraps this in one place. As for now this can stay as-is. *) Your templates extend dashboard templates. Please don't do that (as those templates are not a stable/public interface). Instead extend lava-server templates that work just as well but are stable.
That's a very quick review. I have one more comment.
EXCELLENT WORK :-)
Could you please join the validation team? :-)
Thanks ZK
- general improvements to the flot library for rendering things
- zoom support for graphs
- DB queries
The DB queries is the main thing Zygmunt complained about from the initial code. I now hit the DB with one query to get all the measurements I need it and let it perform the StdDev/Avg[1] calculations. The performance (at least on my laptop) seems pretty good now. The DB query improvement also allowed me to get rid of some ugly code.
I'm hoping going forward I can start doing submissions as code reviews in Gerrit and have someone on this team look at each change.
Let me know what you think - or we can always sit down and chat about it directly next week.
[1]: NOTE: StdDev/Avg functions aren't supported by Sqlite3, so this feature only works with postgres/mysql.
-andy
Thanks for the review. You've taught me about some interesting things in Django. Comments in-line.
On 02/03/2012 03:24 PM, Zygmunt Krynicki wrote:
Hi Andy.
I'm reading the code now. I have some comments. Essentially before this lands you _have_ to add one change, then we can follow up with iterative improvements.
You _have_ to apply south migrations to your application. Without that we will not be able to make reasonable upgrades. Adding migrations later is not an option as there is a bit that is very hard to automate.
Fortunately this is pretty easy:
- Add a dependency on south (install_requires in setup.py)
- Run lava-server manage ... android_benchmark_views_app
schemamigration --initial --auto 3) Inspect the code briefly and add it to version control 4) Push that up for review.
I have a patch ready for review: http://review.android.git.linaro.org/#change,1463
As for the rest:
*) helpers.py, don't use double leading underscore.
http://review.android.git.linaro.org/#change,1464
*) Try keeping the code that relates to your model in the model class, also add a few one liners if you can to document each public method
I added some comments to models.py to help explain things: http://review.android.git.linaro.org/#change,1468
*) extension.py, in def version(), pass the second argument to versiontools.format_version() please pass your main package object (android_benchmark_views_app). This will make it follow git version hashes.
I'm not positive I did what you were wanting, but:
http://review.android.git.linaro.org/#change,1465
*) models.py, BenchmarkRun, each ForeignKey should have a related_name. This name is used when accessing the model field from the "remote" end, then you can navigate back using this name. In general this is required in some cases (when there are clashes, anyway this is beyond the scope of this conversation)
http://review.android.git.linaro.org/#change,1466
*) models.py BenchmarkReport, don't use comments like that. Firs of all, in django it's very very bad to use nullable text/char fields. Instead always use null=False, blank=True - that makes writing code then saner. As for comments. Instead of using a simple text field please consider using django comment system. It _is_ slightly more complicated than using a free-for-all text area (like on launchpad's whiteboard but at the same time you get much more in return. You can read about this here: https://docs.djangoproject.com/en/dev/ref/contrib/comments/
So for now I've just changed the nullable to be false. I'm not sure the django comment system is what we need for this. Basically in this case I intend "comments" to be where you can put a bit of an "executive summary" for the given report. The change I made added some help_text to the field to help convey that.
http://review.android.git.linaro.org/#change,1470
Let me know if you think this is adequate or if the comment system still would be better. It looks easy enough to hook up.
*) models.py both classes, add a get_absolute_url() method based on django permalink system. It's trivial to use, see this document: https://docs.djangoproject.com/en/dev/ref/models/instances/#django.db.models...
http://review.android.git.linaro.org/#change,1467
*) You ship query.flot. While that's okay for now I'd rather see this managed by a dedicated django app that just wraps this in one place. As for now this can stay as-is.
Yeah, that probably makes sense. My versions have some handy fixes that took me a while to come up with, but would be useful to others wanting to do some charting.
We (read I) should probably look into submitting patches for this, but I see so many flot patches floating around that I assume the maintainers probably aren't interested.
*) Your templates extend dashboard templates. Please don't do that (as those templates are not a stable/public interface). Instead extend lava-server templates that work just as well but are stable.
http://review.android.git.linaro.org/#change,1469
That's a very quick review. I have one more comment.
EXCELLENT WORK :-)
I cheated and tried to copy all the great work your team has done.
Zygmunt, have you had a chance to review the changes that Andy pointed at below? It's a bit different from the merge proposal process we are accustomed to, but pretty easy to sort out. Andy can correct me if I'm wrong, but I think you can just sign in with your linaro google id, setup the account, and review the changes.
If there are no blockers, any chance we can roll this into production tomorrow? Andy has his demo in the afternoon, so it would be nice if we can get him setup by then. He may require staff access at least so that he can access the admintool.
Thanks, Paul Larson
On Fri, Feb 3, 2012 at 3:29 PM, Andy Doan andy.doan@linaro.org wrote:
Thanks for the review. You've taught me about some interesting things in Django. Comments in-line.
On 02/03/2012 03:24 PM, Zygmunt Krynicki wrote:
Hi Andy.
I'm reading the code now. I have some comments. Essentially before this lands you _have_ to add one change, then we can follow up with iterative improvements.
You _have_ to apply south migrations to your application. Without that we will not be able to make reasonable upgrades. Adding migrations later is not an option as there is a bit that is very hard to automate.
Fortunately this is pretty easy:
- Add a dependency on south (install_requires in setup.py)
- Run lava-server manage ... android_benchmark_views_app
schemamigration --initial --auto 3) Inspect the code briefly and add it to version control 4) Push that up for review.
I have a patch ready for review: http://review.android.git.linaro.org/#change,1463
As for the rest:
*) helpers.py, don't use double leading underscore.
http://review.android.git.linaro.org/#change,1464
*) Try keeping the code that relates to your model in the model class, also add a few one liners if you can to document each public method
I added some comments to models.py to help explain things: http://review.android.git.linaro.org/#change,1468
*) extension.py, in def version(), pass the second argument to versiontools.format_version() please pass your main package object (android_benchmark_views_app). This will make it follow git version
hashes.
I'm not positive I did what you were wanting, but:
http://review.android.git.linaro.org/#change,1465
*) models.py, BenchmarkRun, each ForeignKey should have a related_name. This name is used when accessing the model field from the "remote" end, then you can navigate back using this name. In general this is required in some cases (when there are clashes, anyway this is beyond the scope of this conversation)
http://review.android.git.linaro.org/#change,1466
*) models.py BenchmarkReport, don't use comments like that. Firs of all, in django it's very very bad to use nullable text/char fields. Instead always use null=False, blank=True - that makes writing code then saner. As for comments. Instead of using a simple text field please consider using django comment system. It _is_ slightly more complicated than using a free-for-all text area (like on launchpad's whiteboard but at the same time you get much more in return. You can read about this here: https://docs.djangoproject.com/en/dev/ref/contrib/comments/
So for now I've just changed the nullable to be false. I'm not sure the django comment system is what we need for this. Basically in this case I intend "comments" to be where you can put a bit of an "executive summary" for the given report. The change I made added some help_text to the field to help convey that.
http://review.android.git.linaro.org/#change,1470
Let me know if you think this is adequate or if the comment system still would be better. It looks easy enough to hook up.
*) models.py both classes, add a get_absolute_url() method based on django permalink system. It's trivial to use, see this document:
https://docs.djangoproject.com/en/dev/ref/models/instances/#django.db.models...
http://review.android.git.linaro.org/#change,1467
*) You ship query.flot. While that's okay for now I'd rather see this managed by a dedicated django app that just wraps this in one place. As for now this can stay as-is.
Yeah, that probably makes sense. My versions have some handy fixes that took me a while to come up with, but would be useful to others wanting to do some charting.
We (read I) should probably look into submitting patches for this, but I see so many flot patches floating around that I assume the maintainers probably aren't interested.
*) Your templates extend dashboard templates. Please don't do that (as those templates are not a stable/public interface). Instead extend lava-server templates that work just as well but are stable.
http://review.android.git.linaro.org/#change,1469
That's a very quick review. I have one more comment.
EXCELLENT WORK :-)
I cheated and tried to copy all the great work your team has done.
linaro-validation mailing list linaro-validation@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-validation
On Tue, Feb 7, 2012 at 6:47 AM, Paul Larson paul.larson@linaro.org wrote:
Zygmunt, have you had a chance to review the changes that Andy pointed at below? It's a bit different from the merge proposal process we are accustomed to, but pretty easy to sort out. Andy can correct me if I'm wrong, but I think you can just sign in with your linaro google id, setup the account, and review the changes.
I tried to review it over the weekend on my phone but the UI was spawning new windows all the time in a loop so I deferred. I'm doing the review now. I've never used gerrit in the past. I hope I get it right.
If there are no blockers, any chance we can roll this into production tomorrow? Andy has his demo in the afternoon, so it would be nice if we can get him setup by then. He may require staff access at least so that he can access the admintool.
I'll sync with Andy and try to get this deployed.
Thanks, Paul Larson
On Fri, Feb 3, 2012 at 3:29 PM, Andy Doan andy.doan@linaro.org wrote:
Thanks for the review. You've taught me about some interesting things in Django. Comments in-line.
On 02/03/2012 03:24 PM, Zygmunt Krynicki wrote:
Hi Andy.
I'm reading the code now. I have some comments. Essentially before this lands you _have_ to add one change, then we can follow up with iterative improvements.
You _have_ to apply south migrations to your application. Without that we will not be able to make reasonable upgrades. Adding migrations later is not an option as there is a bit that is very hard to automate.
Fortunately this is pretty easy:
- Add a dependency on south (install_requires in setup.py)
- Run lava-server manage ... android_benchmark_views_app
schemamigration --initial --auto 3) Inspect the code briefly and add it to version control 4) Push that up for review.
I have a patch ready for review: http://review.android.git.linaro.org/#change,1463
As for the rest:
*) helpers.py, don't use double leading underscore.
http://review.android.git.linaro.org/#change,1464
*) Try keeping the code that relates to your model in the model class, also add a few one liners if you can to document each public method
I added some comments to models.py to help explain things: http://review.android.git.linaro.org/#change,1468
*) extension.py, in def version(), pass the second argument to versiontools.format_version() please pass your main package object (android_benchmark_views_app). This will make it follow git version hashes.
I'm not positive I did what you were wanting, but:
http://review.android.git.linaro.org/#change,1465
*) models.py, BenchmarkRun, each ForeignKey should have a related_name. This name is used when accessing the model field from the "remote" end, then you can navigate back using this name. In general this is required in some cases (when there are clashes, anyway this is beyond the scope of this conversation)
http://review.android.git.linaro.org/#change,1466
*) models.py BenchmarkReport, don't use comments like that. Firs of all, in django it's very very bad to use nullable text/char fields. Instead always use null=False, blank=True - that makes writing code then saner. As for comments. Instead of using a simple text field please consider using django comment system. It _is_ slightly more complicated than using a free-for-all text area (like on launchpad's whiteboard but at the same time you get much more in return. You can read about this here: https://docs.djangoproject.com/en/dev/ref/contrib/comments/
So for now I've just changed the nullable to be false. I'm not sure the django comment system is what we need for this. Basically in this case I intend "comments" to be where you can put a bit of an "executive summary" for the given report. The change I made added some help_text to the field to help convey that.
http://review.android.git.linaro.org/#change,1470
Let me know if you think this is adequate or if the comment system still would be better. It looks easy enough to hook up.
*) models.py both classes, add a get_absolute_url() method based on django permalink system. It's trivial to use, see this document:
https://docs.djangoproject.com/en/dev/ref/models/instances/#django.db.models...
http://review.android.git.linaro.org/#change,1467
*) You ship query.flot. While that's okay for now I'd rather see this managed by a dedicated django app that just wraps this in one place. As for now this can stay as-is.
Yeah, that probably makes sense. My versions have some handy fixes that took me a while to come up with, but would be useful to others wanting to do some charting.
We (read I) should probably look into submitting patches for this, but I see so many flot patches floating around that I assume the maintainers probably aren't interested.
*) Your templates extend dashboard templates. Please don't do that (as those templates are not a stable/public interface). Instead extend lava-server templates that work just as well but are stable.
http://review.android.git.linaro.org/#change,1469
That's a very quick review. I have one more comment.
EXCELLENT WORK :-)
I cheated and tried to copy all the great work your team has done.
linaro-validation mailing list linaro-validation@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-validation
linaro-validation@lists.linaro.org
-
Andy Doan -
Paul Larson -
Zygmunt Krynicki