search

aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
414 stars 185 forks source link

CLI: Optional sorting for `verdi code export` #6345

Closed GeigerJ2 closed 1 month ago

GeigerJ2 commented 2 months ago

When running verdi code export, the generated yaml file is automatically sorted. However, the resulting alphabetical ordering is not what is typically found in the code yaml files in the aiida-code-registry and aiida-resource-registry. This PR turns the sorting off by default, and provides a command-line option to turn on the sorting.

If we don't find any other use cases for this flag, I'd also be fine to just turn off the yaml sorting alltogether, as I don't think this is the format that we'd usually want in the yaml file?

I found the previous behavior to be a slight annoyance, while setting up some unfamiliar codes by exporting them from an existing archive and modifying the resulting yaml files accordingly.

GeigerJ2 commented 2 months ago

Sry, forgot to update the tests. Will do so if this change is desired.

sphuber commented 2 months ago

Even if you remove the explicit sorting, the actual order is not going to be the same as those on the registries, right? It would depend on whether the pydantic.BaseModel.fields have any particular ordering.

GeigerJ2 commented 2 months ago

Even if you remove the explicit sorting, the actual order is not going to be the same as those on the registries, right? It would depend on whether the pydantic.BaseModel.fields have any particular ordering.

That's a good point, which I didn't really consider. I just tested it, and it seems that when calling code.Model.model_fields as is done in the export function, they are returned in the same order as they are defined under class Model(BaseModel) of AbstractCode. This is then followed by the additional fields added by the derived classes (e.g. computer and filepath_executable for InstalledCode), while unset, optional fields are excluded in verdi code export.

When changing the ordering of the fields in the yaml file, importing the code, and then exporting it again, the initial, modified ordering from the yaml file is not retained, but the fields are automatically sorted, as described in the paragraph above. I think this should actually be the intended behavior, as it defines a standard ordering that derives from the actual implementation. If the sorting is different for yaml files in the registry, that should be the responsibility of the person who uploaded it there, and not the code export functionality of AiiDA. In any case, I don't think we can guarantee 100% the same sorting all the time, but at least with sort_keys=False in yaml.dump, append_text wouldn't be shown as the first field, which is something I found quite confusing with the previous behavior.

sphuber commented 2 months ago

but at least with sort_keys=False in yaml.dump, append_text wouldn't be shown as the first field, which is something I found quite confusing with the previous behavior.

And perhaps others will find it surprising if the order changes and/or is not alphabetical. In the end though, these are just configuration files that are machine readable. Just thought that this change wouldn't be that impactful especially if the order is still not necessarily going to be uniform anywhere. But I don't really care that much about it going in either, so let's go ahead and add this change. Could you just please add a test for the option. And since this is (for now) just used in verdi code export not sure if it deserves a generic option. May as well just define it directly on the command for now

GeigerJ2 commented 2 months ago

And perhaps others will find it surprising if the order changes and/or is not alphabetical.

Yeah, I guess this just boils down to personal preference in the end. But at least having the option will be nice. Eventually, also having verdi computer export[^1] would be nice, so it might be an option there, as well. But for now it could just be specific for these two commands.

[^1]: Ofc being aware of the different implementation of a Computer as compared to a Code, and the need for setup and configure as separate steps. Might have a first look at this if I get around.

GeigerJ2 commented 1 month ago

Alright, this should be ready for merge. I actually changed the option from --sort-outputs to --unsorted, with the default as false, so that people don't get surprised by the behavior suddenly changing. For testing via file_regression.check(), I added a new function, as I wasn't sure how to modify the existing test_code_export to check for the unsorted output.

GeigerJ2 commented 1 month ago

As always, thanks for your help and assistance @sphuber, very much appreciated :pray: