Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

some issues with two examples from the tutorials directory #624

Closed
wants to merge 3 commits into from
Closed

Conversation

v0lker
Copy link

@v0lker v0lker commented Aug 30, 2018

tutorial/00_LisaInANutshell: should be trying to set ENERGY_AWARE scheduling as root, otherwise permission will be denied (i haven't tested the rest of the file)

tutorial/UseCaseExamples_SchedTuneAnalysis: was not using the path that was set to load the trace file and the ASCII trace file is missing, therefore need to generate it

there are more issues, at least with the latter one.

Copy link
Contributor

@derkling derkling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for the fixes!

Just a couple of nit-picks:

  • as a general comment, we try to use a common prefix for patches which it's a sort of tag representing the main subsystem interested. Thus, for these patches, something like ipynb: tutorial: should do the job (git lg on the file is usually a good reference).
  • for the last patch, let's avoid commit header ending with "...", just use one single line for the commit title and (when useful) add one/more sentence in the commit message body

"\n",
"# Load the platform information\n",
"with open('../../results/SchedTuneAnalysis/platform.json', 'r') as fh:\n",
"with open(res_dir + os.sep + 'platform.json', 'r') as fh:\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of hardcoded concatenations, we preferably use:

os.path.join(res_dir, 'platform.json')

@v0lker
Copy link
Author

v0lker commented Sep 7, 2018

i hope i addressed the concerns - ?

@v0lker
Copy link
Author

v0lker commented Oct 24, 2018

it's been two months and nobody seems to be using the tutorial. in that case it's probably better if the tutorials were deleted?

@valschneider
Copy link
Contributor

Hi @v0lker,

Sorry about that. Notebooks are quite a pain to maintain because you can't really add a Travis job to make sure they still work (you can translate them to a Python script, but then you have nothing to check that e.g. the plots got rendered correctly).

Some of those Notebooks serve a dual purpose - both API documentation and tutorial/example code. This stems from the fact that currently our API documentation is in a poor shape, and that sometimes writing pure code is simpler than writing text...

A non negligible work that remains to be done on https://github.com/ARM-software/lisa/tree/next is to do a summer cleaning of those notebooks - remove redundant ones, remove those serving as pure API documentation, and only keep a few useful example ones. The rest should live in the documentation, and that can actually be CI-tested via https://docs.python.org/3.5/library/doctest.html.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants