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

feat: yaml worflow #173

Merged
merged 7 commits into from
Jun 5, 2020
Merged

feat: yaml worflow #173

merged 7 commits into from
Jun 5, 2020

Conversation

mihai-sysbio
Copy link
Member

@mihai-sysbio mihai-sysbio commented May 27, 2020

Main improvements in this PR:

This PR addresses all yaml indentation issues from #169 and adds a GitHub Action to run yamllint on PRs to master and devel, and pushes to devel. The yamllint configuration {extends: default, rules: {line-length: disable}}, which, unlike extends: relaxed means most problems will result in errors, as per the documentation. The line-length is disabled because it's unlikely this will be addressed.
I've ran testYamlConversion.m on the new yaml but nothing more.

Edit: Cobrapy import has also been added to the workflow.

I hereby confirm that I have:

  • Tested my code on my own computer for running the model
  • Selected devel as a target branch

@haowang-bioinfo
Copy link
Member

@mihai-sysbio it's very nice to have GH actions checks!

@haowang-bioinfo haowang-bioinfo self-requested a review May 27, 2020 07:02
@haowang-bioinfo haowang-bioinfo linked an issue May 27, 2020 that may be closed by this pull request
3 tasks
@haowang-bioinfo
Copy link
Member

There is still one warning reported from yamllint and maybe worth to fix it here?
1:1 warning missing document start "---" (document-start)

@mihai-sysbio
Copy link
Member Author

I saw that, but was afraid to fix it - I'm not sure if all yaml parsers can handle ---.

@JonathanRob
Copy link
Collaborator

It looks like the changes to the yaml indentation (d075b07) broke compatibility with cobrapy, unfortunately.

@mihai-sysbio
Copy link
Member Author

mihai-sysbio commented May 27, 2020

Compatibility with cobrapy has been restored, and checking this through a model import is now also part of the same GH action as yamllint. This commit also adds in --- at the top of the yaml file.

Copy link
Member

@haowang-bioinfo haowang-bioinfo left a comment

Choose a reason for hiding this comment

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

Excellent contribution @mihai-sysbio

@haowang-bioinfo haowang-bioinfo merged commit d5c8ddf into devel Jun 5, 2020
@haowang-bioinfo haowang-bioinfo deleted the feat/yaml-workflow branch June 5, 2020 13:53
@mihai-sysbio
Copy link
Member Author

Thanks @Hao-Chalmers and @JonathanRob for the feedback in resolving the PR.

@BenjaSanchez
Copy link

BenjaSanchez commented Jul 20, 2020

@Hao-Chalmers @mihai-sysbio I found this PR and I wonder why are all these modifications to writeHumanYaml.m not a part of RAVEN's writeYaml.m? Moreover, any reason to have writeHumanYaml.m at all and not directly rely on writeYaml.m? That way all these changes could benefit all repos that use writeYaml.m, for instance it would resolve failing tests at SysBioChalmers/yeast-GEM#236

@mihai-sysbio
Copy link
Member Author

As you're suggesting @BenjaSanchez I also think it's better to fix problems at the source, in this case to apply the fixes directly to RAVEN. However, I'm not sure why writeHumanYaml came to be - maybe @JonathanRob remembers?

@haowang-bioinfo
Copy link
Member

@BenjaSanchez the development of writeHumanYaml was triggered by this issue #71.

@JonathanRob
Copy link
Collaborator

The initial form of writeHumanYaml was super specific to Human-GEM and its parsing by Metabolic Atlas, so at that time it didn't make sense to apply the changes directly to RAVEN. Now that it has developed more, maybe it has reached a point that it is feasible to work toward merging it with the RAVEN function.

@mihai-sysbio
Copy link
Member Author

That's my mistake then - pushing for the development of a custom yaml export instead of relying upon the RAVEN export with potential modifications. I fully support the merging of the two.

@haowang-bioinfo
Copy link
Member

I also support the merging, which does appear to the right time point after evolvements.

@BenjaSanchez
Copy link

BenjaSanchez commented Jul 20, 2020

@JonathanRob @mihai-sysbio @Hao-Chalmers sounds great! Anyone from the human-GEM / Metabolic Atlas side that would like to take up this? I'm not familiar with all these additional Metabolic Atlas-specific fields... I can imagine that a metabolicAtlas flag that defaults to false would be the most sensible way of doing it? I opened SysBioChalmers/RAVEN#309 that already adds quotes to strings, so work could be continued from there?

@haowang-bioinfo
Copy link
Member

haowang-bioinfo commented Jul 20, 2020

I can start to work on this. Maybe also migrate importYaml to RAVEN, in addition to writeYaml?

@haowang-bioinfo
Copy link
Member

@mihai-sysbio I do not view #71 as a mistake, but improvement or enhancement instead.

@BenjaSanchez the first issue about writeYaml migration is how to deal with metadata in RAVEN? The metadata info will add two new fields version and description to model spec and adjust existing annotation field.

@BenjaSanchez
Copy link

@Hao-Chalmers are you referring to the 1st field in the human-GEM yml metaData? In practice this should not be a problem for writeYaml, as you could choose to write it only if the information is available in the model structure under your current format, and otherwise skip it, keeping compatibility with models that don't have that data.

Having said that, is the standard for metaData documented somewhere? Perhaps these missing field should become a part of RAVEN_structure_fields.csv? Although I do see a description field already matching there, what's the difference between the two?

@haowang-bioinfo
Copy link
Member

@BenjaSanchez will start a RAVEN issue to address this.

@mihai-sysbio
Copy link
Member Author

@haowang-bioinfo
Copy link
Member

@mihai-sysbio both #77 and #107 should be closed, and I did.

There is pending conclusions to #311, which seems to be relevant to standard-GEM as well.

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.

YAML format issues in modelFiles/yml/HumanGEM.yml
4 participants