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

Refactor PURL system scripts #494

Merged
merged 22 commits into from
Mar 7, 2019

Conversation

jamesaoverton
Copy link
Member

As part of the OBO Technical Services Grant, I've asked @lmcmicu to refactor the scripts for the PURL system. Goals: simplify the code to reduce long-term maintenance burden and expand the number of potential maintainers; improve build performance. Changes:

  • Various translate_*.py scripts have been combined into a single translate_yaml.py script.
  • Simplify execution: translate_yaml.py runs against the config directory, instead of calling three scripts against every config file. I think this is simpler, and performance is much better.
  • Drop kwalify in favour of jsonschema in translate_yaml.py.
  • Drop travis CLI client in favour of safe-update.py script (basic REST API calls).
  • Drop ruby dependency.
  • Simplified Makefile (prefer Python for tricky bits).
  • Improve tests.

This is critical code, so we want to be careful. I reviewed the code and ran the test suite. I'm going to spend more time convincing myself that the resulting .htaccess files are equivalent. Then I'll ask for other devs to review.

Michael Cuffaro and others added 21 commits February 6, 2019 16:32
…he Makefile logic as well as some or all of the existing python scripts
…files case-insensitively, longer names first (to ensure no regex conflicts)
Some project files have been changed since the refactoring work began. To properly test that
the refactoring does not break anything we need to have these changes in the branch.
… remove empty example_terms and entries arrays from dpo.yml and xao.yml
@jamesaoverton jamesaoverton changed the title WIP: Refactor scripts WIP: Refactor PURL system scripts Feb 27, 2019
@jamesaoverton
Copy link
Member Author

See #105 "Code review".

@jamesaoverton
Copy link
Member Author

I compared the output of branch with the output master at commit bd9e470 (where it branched from). Each www/obo/*/.htaccess file is identical. The order of entries in www/obo/.htaccess is different, but the files are identical when sorted. So I'm convinced that the output of the new code is identical to the output of the old code.

(And time make all dropped from 52.4s to 1.8s on my machine.)

I'd appreciate a few more pairs of eyeballs on this. Volunteers?

@cmungall cmungall requested a review from kltm February 28, 2019 16:36
@kltm
Copy link
Contributor

kltm commented Feb 28, 2019

On review, this is a large change to code I am unfamiliar with; nothing seems obviously wrong, but I would be unlikely to be able to catch mistakes.
I might suggest that sometime in the future that there is a blessed dev branch of the code that deploys either under a separate machine or separate virtual host set to get a look at how changes might look when going live or redeploying.

Copy link
Contributor

@kltm kltm left a comment

Choose a reason for hiding this comment

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

As #494 (comment)
There can always be rollbacks if there are issues.

@beckyjackson
Copy link
Contributor

I ran make clean all and the first thing I saw was a bunch of warnings:

WARNING: In project 'CHEBI', product: 'chebi.owl.gz' does not end with '.owl' or '.obo'
WARNING: In project 'CHEBI', product: 'chebi.obo.gz' does not end with '.owl' or '.obo'
WARNING: Base URL '/obo/cheminf' must end with 'CHEMINFO', not 'cheminf'
WARNING: idspace: CHEMINFO does not match filename config/cheminf.yml
WARNING: In project 'ENVO', product: 'envo.json' does not end with '.owl' or '.obo'
WARNING: In project 'GO', product: 'go.json' does not end with '.owl' or '.obo'
WARNING: In project 'MONDO', product: 'mondo.json' does not end with '.owl' or '.obo'
WARNING: In project 'OBA', product: 'oba.json' does not end with '.owl' or '.obo'
WARNING: In project 'PATO', product: 'pato.json' does not end with '.owl' or '.obo'
WARNING: In project 'RO', product: 'ro.html' does not end with '.owl' or '.obo'

Can these be removed or are these important messages? It seems fine to allow file extensions other than OWL and OBO...

I tested some terms and products from different ontologies and it all looks like it's working like it's supposed to! It's also MUCH faster than the old system (in terms of running the tests and starting the system)

@jamesaoverton
Copy link
Member Author

Thanks for reviewing the code.

I want those warnings in there. We need to be more strict about products and make some decisions about what is and is not allowed. json is fine. I'm not sure about html. See #434, #421. The IDSPACE warning is legitimate.

@jamesaoverton jamesaoverton changed the title WIP: Refactor PURL system scripts Refactor PURL system scripts Mar 7, 2019
@jamesaoverton jamesaoverton merged commit fbd5b4c into OBOFoundry:master Mar 7, 2019
@jamesaoverton
Copy link
Member Author

After triple checking everything, I just merged this an manually updated the PURL server. Automated tests are passing. I did some manual tests and everything seemed to work.

@OBOFoundry/purl-admin If you notice anything weird, let me know!

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