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

fix-add a error report in the import/export cycle #629

Merged
merged 5 commits into from
Jun 16, 2023

Conversation

feiranl
Copy link
Collaborator

@feiranl feiranl commented Jun 12, 2023

Main improvements in this PR:

This PR is to fix only the bug found in Yaml file, as reported in #628. In the meantime, another PR #635 is to fix error of GH workflow test

@feiranl
Copy link
Collaborator Author

feiranl commented Jun 12, 2023

@haowang-bioinfo Could you help to fix this? I add an error catch, but the task still succeed.

@haowang-bioinfo
Copy link
Member

haowang-bioinfo commented Jun 12, 2023

I propose a error message to fix the bug.

looks good - the actual cause is now fixed in 182010d

Regarding the pass of yamlConversion task, it is presumably due to using previously cached model by the GitHub action cloud server. So a probable solution might be "forcing" GH action to use freshly loaded Yaml model every time. What do you think @mihai-sysbio?

One practical solution (that I used sometimes) could be locally running testYamlConversion function to make sure everything is ok

- The changes in this commit are to move subsystem reading code into the "case" of reading confidence_score. This optimise the logic of loading subSystems according the Yaml file structure.
@mihai-sysbio
Copy link
Member

I'm confused - the test is now successful. Is that right?

My idea was to start by fixing the test first, so that we can confirm that the test will trigger a failure of the GH workflow. Only after that the yaml problem would be fixed, and the re-run of the workflow would then pass.

@mihai-sysbio mihai-sysbio deleted the fix-importExportCycle branch June 13, 2023 01:23
@mihai-sysbio
Copy link
Member

I renamed the branch to fix/importExportCycle, which then triggered a closure of the PR and deletion of the branch.

@feiranl
Copy link
Collaborator Author

feiranl commented Jun 13, 2023

It is not ready for closing. There is another bug I identified in importYaml.

the import and export cycle changes the yml file.
MicrosoftTeams-image

@mihai-sysbio mihai-sysbio mentioned this pull request Jun 13, 2023
3 tasks
@mihai-sysbio
Copy link
Member

It is not ready for closing

Absolutely! It was not the intention to close the PR, sorry about that. Normally branch renaming is seamless, but apparently when a PR is already open, GitHub will close it. Let's continue in #635.

@haowang-bioinfo
Copy link
Member

There is another bug I identified in importYaml.
the import and export cycle changes the yml file.

yes indeed, working on this now

@haowang-bioinfo
Copy link
Member

haowang-bioinfo commented Jun 13, 2023

There is another bug I identified in importYaml.
the import and export cycle changes the yml file.

yes indeed, working on this now

this bug is located from Yaml file line 307295: - metabolties: !!omap, where there is typo that supposed to be - metabolites: !!omap. This particular bug was introduced by #606. This type of bug probably can be detected by Yaml lint checker by providing a finite set of keys, from which any unknown keys could thus be reported.

another bug introduced from #606 was the met id MAM1261m in the line 307296 - the middle 0 is missing and correct value should be MAM01261m

- This commit includes fixing typos, and reverting back to original way of extracting subsystem values
@haowang-bioinfo
Copy link
Member

this PR is reopened for fixing yaml import/export cycle only, while the issue of GH workflow test will be fixed in #635

@feiranl
Copy link
Collaborator Author

feiranl commented Jun 16, 2023

LGTM! The import and export cycle works fine now

@haowang-bioinfo haowang-bioinfo merged commit 1ee665e into develop Jun 16, 2023
@haowang-bioinfo haowang-bioinfo mentioned this pull request Jun 17, 2023
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.

3 participants