SwissDataScienceCenter / renku-project-template

A repository for default Renku project templates.
https://renkulab.io
4 stars 20 forks source link

renku.ini documentation in comments #153

Closed RichardJActon closed 1 year ago

RichardJActon commented 1 year ago

Adds documentation to the renku.ini files in comments with a link to the docs on renku.ini for more details

rokroskar commented 1 year ago

Thanks @RichardJActon for this PR! It doesn't seem to play super nicely together with using the UI for making changes to the project settings - this is the diff I get if I just make a simple configuration change via the UI:

image

RichardJActon commented 1 year ago

Hmm yes looks like this simple approach is not going to work as interacting with it with the web UI just seems to clobber the file and replace it with new contents. I'd guess that maybe the code which writes the new session config is either just writing a fresh config with strings rather than say using a .ini parser editing the contents of the appropriate fields in the resulting data structure representing the ini AST and then writing it back. Or that the ini parser, if one is being used, is not respecting comments when the new ini is written. I couldn't find in the other project repos where this happens with a quick search for renku.ini do you know if this is likely to be in renku, renku-python, or renku-ui @rokroskar so I can take a look and understand the reason for this behaviour and if there is a way around it?

This file from renku-python seems to be responsible for some but possibly not all of the edits to renku.ini https://github.com/SwissDataScienceCenter/renku-python/blob/10d8cb7f18a8deac1de829f277fea951621b5ca3/renku/ui/cli/config.py

rokroskar commented 1 year ago

I'm not entirely sure, to be honest - @lorenzo-cavazzi can you help here? How do the settings get set in the UI?

lorenzo-cavazzi commented 1 year ago

From the UI, we invoke the POST /renku/config.set API that ultimately use the same function invoked by the CLI counterpart renku config set already linked by @RichardJActon . Even if the "renku.ini" file was not intended to be manually modified, it would be nice to add some comments -- at least the reference to the documentation. I don't know the effort required on the backend to modify the logic so that it preservers to comments on changes. Let me forward the ping to @Panaetius (sorry for bouncing that back to someone else :stuck_out_tongue: )

rokroskar commented 1 year ago

I agree, I think some in-line docs would be very helpful though I guess we didn't intend people to be modifying that file manually?

lorenzo-cavazzi commented 1 year ago

I guess the link to the docs should be fine -- there, users would learn how to modify the settings in a safe way

RichardJActon commented 1 year ago

Just having it write out a single comment line with the link to the docs would probably be fine. The technically 'proper' way of doing it of parsing the ini editing the AST and writing it back whenever the the GUI makes a change should allow GUI and manually edits to co-exist which is why I brought it up but yeah it is probably a disproportionate amount of work for some inline docs.

rokroskar commented 1 year ago

The renku-python code does use the config editor which parses the ini - I wonder if there is some flag that keeps comments intact...

RichardJActon commented 1 year ago

Ah OK that might make things easier wasn't sure if that was a library or just a call to a function/method elsewhere - not much of a python guy, I can checkout the docs to that package. Where are the default values defined though they don't seem to be in that file?

RichardJActon commented 1 year ago

OK so from a look at the docs, specifically this secion https://docs.python.org/3.8/library/configparser.html#customizing-parser-behaviour and this SO question https://stackoverflow.com/questions/12853628/write-comment-in-config-files-with-python looks like this library does not have fully featured comments handling - disappointing. However we might be able to hack in a comment line with something like this:

config.set('interactive', '; ', 'See: https://renku.readthedocs.io/en/latest/reference/templates.html#renku for more info on renku.ini')

Only works within a section so can't be the first line in the file and you have to make the key unique so you have to resort to this sort of thing if you want more lines:

config.set('interactive', ';', ' The space is here now')
config.set('interactive', '; Can', ' I manage a 3rd line')

This also means these won't be read properly so no happy GUI/manual editing co-existance for the comments :(

Panaetius commented 1 year ago

I don't think we have to be able to set comments using config.set, manually editing is good enough for that. Simply preserving comments should be enough.

There's a hack with manipulating configparser to think comments are proper values, as mentioned here but sounds a bit too hacky for my tastes.

Or use https://github.com/pyscaffold/configupdater which is a drop-in replacement for the standard library configparser but keeps comments intact. Though I'm always leery of adding even more dependencies to the renku-python library, it already has a lot. But still something worth considering.

And just for fun, there's a really old issue for this.

RichardJActon commented 1 year ago

Ah looks like configupdater would solve it then though if you don't want an extra dependency we could just manually prepend a comment line that links to the docs to the beginning of the file in the write_config function.

def write_config(filepath, config):
    """Write config value to a specified path."""
    with open(filepath, "w+") as file:
        file.write("; See: https://renku.readthedocs.io/en/latest/reference/templates.html#renku for more info on renku.ini\n")
        config.write(file)

(I found the defaults file and figured out where it was read in once I read core/config.py)