conda-incubator / conda-store-ui

conda-store-ui is a frontend for conda-store powered by react
https://conda-incubator.github.io/conda-store-ui/
BSD 3-Clause "New" or "Revised" License
13 stars 18 forks source link

[ENH] - Add ability to comment environment.yml #272

Closed kcpevey closed 1 week ago

kcpevey commented 1 year ago

Feature description

I like to add comments to my envionment.yml to help with organization. For example, I might want groups packages for tests, documentation, ui apps, and core dependencies. I also like to comment individual lines with some explanation such as why this package is required, what part of the code needs it, or why it is pinned.

This is an example of the style and example comments I find helpful to use:

image

However, when I put this into conda store and create a new env, conda-store wipes all of my comments away and this is what I'm left with:

image

I'd like the ability to preserve my comments.

Value and/or benefit

People could add some much needed comments to their environment files on conda-store.

Anything else?

No response

trallard commented 1 year ago

I am guessing this is related to https://github.com/conda-incubator/conda-store-ui/issues/274 - since the # is not in the allowlist there might be some validation/sanitization happening to find and remove

kcpevey commented 1 year ago

I dont think its related to #274. That issue errors on invalid characters in the ui description which has to do with conda-store-ui validation. This issue doesn't error, it just looses information (# comments and extra whitespace) that are placed in the env spec.

I think its just a matter of roundtripping a yaml to include comments can be tricky. I don't know the backend, but I'd guess its using pyyaml which simply ignores these things, but we could switch to ruamel and keep the comments.

trallard commented 1 year ago

Ah I completely forgot we were running on pyyaml - I always forget also the differences between the two, so it would be good to have a look at how replacing one for another might impact the project overall.

nkaretnikov commented 1 year ago

I'll be working on this. First sub-task here is to identify the impact of making this switch.

One thing that was mentioned about pyyaml during weekly sync is that it might have better YAML support compared to ruamel.

costrouc commented 1 year ago

This issue is difficult due to the way that the environment.yaml is being stored currently as a json datastructure on the database.

Currently environment specifications are being stored in the database as a json structure. Additionally the specification in the schema is being stored as a dict.

In order to properly store comments we need to be storing the text and not the data representation. I agree that long term this needs to be done but I do think that this touches many aspects of the code and would impact the api. Additionally this issue would ensure that the order that users enter in text would be preserved (currently it isn't entirely).

Regarding ruamel vs. pyyaml. The usage of pyyaml by me in the past was honestly not understanding the difference between the two parser well. There are a long list of reasons we should be using ruamel: yaml 1.2 support, concrete syntax parsing, ability to insert comments and data in specific parts of the yaml. I think it is a no brainer to create an PR to transition to ruamel for everything.

At a minimum this PR will require a database migration.

Questions:

I think this PR is two lines of work:

nkaretnikov commented 1 year ago

My 2c:

For those wanting to follow the code with regard to YAML -> DB interaction. Here's an example of how env editing is implemented:

@router_ui.get("/environment/{namespace}/{environment_name}/edit/")
async def ui_edit_environment(
...
    # Get data from the DB
    environment = api.get_environment(db, namespace=namespace, name=environment_name)
...
    context = {
...
        # Finds matching specification and converts it back to YAML
        "specification": yaml.dump(environment.current_build.specification.spec),
...
class Specification(Base):
    """The specifiction for a given conda environment"""
...
    spec = Column(JSON, nullable=False)
...

Action items: next week, I'm planning to submit the following PRs, so we could discuss the details:

UPD (5 Sep 2023): Tania and I agreed to have a follow-up discussion, starting from 11 Sep 2023.

trallard commented 1 year ago

@nkaretnikov - Please hold off on further PRs until we assess impact vs. effort.

trallard commented 1 week ago

This was too much effort vs gain so will close as not planned