-
Notifications
You must be signed in to change notification settings - Fork 117
[doc] Make basic configuration example more intuitive w.r.t. to modules systems #1890
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
[doc] Make basic configuration example more intuitive w.r.t. to modules systems #1890
Conversation
A laptop would generally not have modules, so saying that explicitly will help highlight to new users that a module setting can be useful. There is a modules setting below for daint, but at least one user skimmed over the more complex case. The line numbers for the literal includes need to be bumped if they are greater than 15, and a range including 15 is shown, then the highlighting can need a bump. Note that the latter index within the range shown, so don't always need a bump. Fixes #1887
|
Can I test this patch? |
|
I'd proof-read this if I could. Is the build docs artifact online somewhere, or would I have to build it myself? |
jjotero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a bit of a different approach to state this in a clearer way. I'm not convinced that adding the modules_system option is the best way for this here, since this is an optional parameter and it already defaults to nomod. I feel that tutorials should be kept as light as possible.
What about just expanding on this in the modules_system bullet instead? (see the comment below)
| 'name': 'catalina', | ||
| 'descr': 'My Mac', | ||
| 'hostnames': ['tresa'], | ||
| 'modules_system': 'nomod', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'modules_system' is not required and it defaults to 'nomod', so I think this should not be included in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is true, but a user of the tutorials typically doesn't know that. They probably don't even know that that setting up modules is a thing they might want to do (see #1887) but by being mildly redundant they might get the clue that if they want modules support then they need to set things up.
If util.find_modules gave an error when modules_system was nomod then there'd have been a way to find out that my problem lay in configuration, and not in invalid construction of my test parameters.
The next example configuration on daint does have modules_system, but also a lot of partition complexity. When I read the docs, I knew I didn't need the latter for running on my dedicated node, so never saw that the example for modules_system was also mentioned there.
There is text lower down that mentions modules_system but it's a bit buried in a section about partitions, so again I missed it.
|
I am also of the opinion to keep the tutorials as light as possible and this was the initial principle. I would go for a warning issued by |
vkarak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a PR with the necessary fixes to your branch: https://github.com/ENCCS/reframe/pull/1
|
Replaced by #1932 |
A laptop would generally not have modules, so saying that explicitly
will help highlight to new users that a module setting can be
useful. There is a modules setting below for daint, but at least one
user skimmed over the more complex case.
The line numbers for the literal includes need to be bumped if they
are greater than 15, and a range including 15 is shown, then the
highlighting can need a bump. Note that the latter index within the
range shown, so don't always need a bump.
Fixes #1887