pombase / fypo

Fission Yeast Phenotype Ontology
Creative Commons Attribution 4.0 International
14 stars 6 forks source link

Repo cleanup post-changes #4328

Closed manulera closed 1 year ago

manulera commented 1 year ago

Hello @kimrutherford and @ValWood.

I have made a bunch of changes in the repository, following some help from the obo people.

I think we no longer need to run the release procedure locally, and we can do it using github actions.

There are two separate actions:

The release action does not commit anything to the repository, and that is because other ontologies do not keep those release artifacts in source control, only the equivalent to fypo-edit. We have already moved fypo.owl and others out of source control because they became too big, so I think it would be safe to remove all the other release artifacts as well, and keep them only as release attachments. However, we have to think if us or someone else is using these files from the repo url and not the release url. We already ammended some urls when we moved fypo.owl out, is there anything else that comes to mind?

In addition, I will take the occasion to remove some files that are no longer needed, e.g. all the .owl and .obo files from the imports folder except for merged_import.owl.

cc: @matentzn does this make sense? Anything else we could clean up since we are doing it?

manulera commented 1 year ago

@matentzn what about the files in this folder https://github.com/pombase/fypo/tree/master/imports should a merged_import.owl be committed there as well? These were updated with the former release pipeline, but the update imports from now seems to only affect this file: https://github.com/pombase/fypo/blob/master/src/ontology/imports/merged_import.owl

Then, we used to get reports in reports/fypo-edit.owl-obo-report.tsv and src/ontology/reports/fypo-edit.owl-obo-report.tsv (identical). What should we do about those?

ValWood commented 1 year ago

Brillaint, thanks for doing this, and thanks @matentzn and @gouttegd for the help!

kimrutherford commented 1 year ago

Great work! That's a big improvement.

manulera commented 1 year ago

Hi @kimrutherford @matentzn.

manulera commented 1 year ago

PS: I will include the warnings as a release file.

matentzn commented 1 year ago

@manulera I think it is sufficient to add a prominent warning on the release notes on GitHub.

Remind people to.

Never ever ever ....

use URLs to refer to data and ontology files on the web.

Always use PURLs and only PURLs!

kimrutherford commented 1 year ago

We have already moved fypo.owl and others out of source control because they became too big, so I think it would be safe to remove all the other release artifacts as well, and keep them only as release attachments

Hi @manulera

Could you explain what you mean by "release attachments". I don't understand.

Kim: Do we use urls of ontologies in the repo that would be moved to the release attachments?

We use these OBO files, which we read directory from a local checkout of the fypo repository:

Never ever ever .... use URLs to refer to data and ontology files on the web.

We definitely don't do that. :100:

Always use PURLs and only PURLs!

Oh dear, we don't do that either. :frowning_face:

matentzn commented 1 year ago

Local checkout is also not reliable, because file locations change within a repo.. if you use purls and curl the ontology from the PURL, than you can be certain that you will always grab the correct file

manulera commented 1 year ago

Hi @matentzn, thanks for your responses. There is one more thing I wanted to ask about. I ran the refresh imports and nothing changed (expected), however this is added to the diff:

https://github.com/pombase/fypo/pull/4331/files

I am not sure what this field is used for. Will it be a problem if we make a release on a different date, say 2023-05-16. Does this mean we always have to run it the same day?

matentzn commented 1 year ago

The release and imports date are very different things! Dont worry, this is exactly as it should be. For the import, the date denotes when the import was last refreshed.

ValWood commented 1 year ago

Hi, I tried to run the new release pipeline for the first time today.

I got an error on refresh imports https://github.com/pombase/fypo/actions/runs/5201493460/jobs/9381726725#step:7:237

@matentzn if you have any info about the error codes please let me know, (@manulera is on vacation so I am stuck until he returns).

I also posted a question on Slack.

Thanks if you can advise.

ValWood commented 1 year ago

From Nico: the problem is that you have not enough memory set tun run the merged import, see https://oboacademy.github.io/obook/howto/odk-setup/#problems-with-memory-important

matentzn commented 1 year ago

Sorry I should have read this better. You did not run on your machine, you ran in GH actions.. See slack, this is a much bigger problem, and I am not sure if it can be solved at all.. is @manulera not there?

ValWood commented 1 year ago

He is in Japan...

ValWood commented 1 year ago

Manu has run it successfully though....

matentzn commented 1 year ago

Yea he may have been lucky though. He must have been close to the memory limit..

kimrutherford commented 1 year ago

I've committed a change which seems to have helped: https://github.com/pombase/fypo/commit/61e36f45f43dc9c919ded475751518c6117eb7e7

The "Refresh imports" Action finished after that: https://github.com/pombase/fypo/actions/runs/5204542287

manulera commented 1 year ago

Bravooo @kimrutherford

ValWood commented 1 year ago

excellent, thanks Kim

ValWood commented 1 year ago

The second part of the pipeline ran OK.