Skip to content

New classifiers with scicrunch RRIDs (⚠️ devops) #2045

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

Merged
merged 42 commits into from
Dec 17, 2020

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Dec 10, 2020

What do these changes do?

This PR extends classifiers to include using K-Core's curated list of research resource identifiers (RRIDs) from scicrunch's RRID Portal.

what are classifiers?

  • Every group has a set of "official" classifiers and its users tag studies with them
  • Classifiers can be defined in two ways:
    1. a static tree (or bundle) that is stored in group_classifiers.c.bundle
    2. using RRIDs from scicrunch.org (see group_classifiers.c.uses_scicrunch )
  • In the UI, classifiers are show in a tree that represents
    1. one-to-one the bundle tree
    2. a dynamic tree built from valid scicrunch RRIDs (currently always flat list)

Design

This has been implemented in the groups apps module of the webserver that uses a new submodule scicrunch to interact with scicrunch.org.

Screen Shot 2020-12-17 at 11 41 05

Related issue/s

How to test

manual testing

Current PR is deployed in my machine (see link in Mattermost) with the following setup

  • Test user (login: [email protected], pwd : test) belongs to NIH-sparc group
  • "NIH-sparc" group uses scicrunch classifiers (i.e. RRID-based classifiers)
  • "Everyone" group has bundle classifiers
  • If a user belongs to several groups all classifiers are shown together

Possible checks

  • classifiers show and info display. Links to scicrunch.org RRIDs are correct
  • can add new RRID to classifiers
  • can assign a classifier to a study
  • ...

unit/integration tests

make devenv
cd services/web/server
make install-dev
make tests

Deployment

  • ⚠️ REQUIRED: setup new environment variables in .env:
SCICRUNCH_API_BASE_URL=https://scicrunch.org/api/1
SCICRUNCH_API_KEY=REPLACE_ME_with_valid_api_key

For API_KEY, create account and get one key for each deploy here here)

Checklist

Next PR.
- [ ] FIX: tests
- [ ] ValidationErrors to 422 Unprocessable Entity
- [ ] Unit tests for the changes exist

  • Did you change any service's API? Then make sure to bundle document and upgrade version (make openapi-specs, git commit ... and then make version-*)
  • Runs in the swarm
  • Documentation reflects the changes
  • New module? Add your github username to .github/CODEOWNERS

@codecov
Copy link

codecov bot commented Dec 10, 2020

Codecov Report

Merging #2045 (3b70881) into master (cb2a0e9) will decrease coverage by 0.4%.
The diff coverage is 53.4%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2045     +/-   ##
========================================
- Coverage    73.2%   72.8%   -0.5%     
========================================
  Files         414     421      +7     
  Lines       15150   15455    +305     
  Branches     1543    1561     +18     
========================================
+ Hits        11099   11256    +157     
- Misses       3652    3798    +146     
- Partials      399     401      +2     
Flag Coverage Δ
integrationtests 63.7% <50.9%> (-0.6%) ⬇️
unittests 67.3% <53.1%> (-0.3%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rc/simcore_postgres_database/models/classifiers.py 100.0% <ø> (ø)
...s/service-library/src/servicelib/client_session.py 0.0% <0.0%> (ø)
.../server/src/simcore_service_webserver/db_models.py 100.0% <ø> (ø)
...r/src/simcore_service_webserver/groups_handlers.py 70.6% <32.5%> (-10.6%) ⬇️
...core_service_webserver/scicrunch/service_client.py 38.0% <38.0%> (ø)
...imcore_service_webserver/scicrunch/scicrunch_db.py 45.7% <45.7%> (ø)
...rc/simcore_service_webserver/groups_classifiers.py 50.0% <50.0%> (ø)
...re_service_webserver/scicrunch/scicrunch_models.py 75.8% <75.8%> (ø)
...ore_service_webserver/scicrunch/submodule_setup.py 84.6% <84.6%> (ø)
...re_postgres_database/models/scicrunch_resources.py 100.0% <100.0%> (ø)
... and 18 more

@pcrespov pcrespov self-assigned this Dec 10, 2020
@pcrespov pcrespov force-pushed the is334/osparc-classifiers branch 2 times, most recently from c5d8b8a to 59dbea6 Compare December 14, 2020 22:55
@pcrespov pcrespov force-pushed the is334/osparc-classifiers branch 3 times, most recently from 1fda721 to 2860114 Compare December 16, 2020 18:52
@pcrespov pcrespov changed the title WIP: Is334/osparc classifiers WIP: Is334/osparc classifiers (⚠️ devops) Dec 16, 2020
@pcrespov pcrespov added a:webserver issue related to the webserver service idea Proposed new functionality that is still not implemented labels Dec 16, 2020
@pcrespov pcrespov force-pushed the is334/osparc-classifiers branch from 2860114 to 7dbbc27 Compare December 17, 2020 01:26
@pcrespov pcrespov marked this pull request as ready for review December 17, 2020 10:44
Copy link
Member

@odeimaiz odeimaiz left a comment

Choose a reason for hiding this comment

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

Tested. Works like a charm 👍

@pcrespov pcrespov force-pushed the is334/osparc-classifiers branch from 7dbbc27 to 876bce2 Compare December 17, 2020 11:49
@pcrespov pcrespov added this to the Alfred_Büchi milestone Dec 17, 2020
Copy link
Contributor

@KZzizzle KZzizzle left a comment

Choose a reason for hiding this comment

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

when I tried to add a new RRID it said success but the new classifier didn't appear on the list until I refreshed the page. Is there a way we can make it automatically appear or maybe a popup telling the user to refresh?

Other than that, bravo!

Copy link
Member

@mguidon mguidon left a comment

Choose a reason for hiding this comment

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

Very nicely done. The only problem I had while testing was that the scicrunch.org server was shaky.

@pcrespov
Copy link
Member Author

Very nicely done. The only problem I had while testing was that the scicrunch.org server was shaky.

scicrunch.org API is sometimes a bit slow to respond but that is why we are keeping the RRIDs cached on our side once somebody has used them. This should help improving the UX

@odeimaiz
Copy link
Member

when I tried to add a new RRID it said success but the new classifier didn't appear on the list until I refreshed the page. Is there a way we can make it automatically appear or maybe a popup telling the user to refresh?

Other than that, bravo!

Coming here
#2060

@pcrespov pcrespov changed the title WIP: Is334/osparc classifiers (⚠️ devops) new osparc classifiers with scicrunch RRIDs (⚠️ devops) Dec 17, 2020
@pcrespov pcrespov changed the title new osparc classifiers with scicrunch RRIDs (⚠️ devops) New osparc classifiers with scicrunch RRIDs (⚠️ devops) Dec 17, 2020
@pcrespov pcrespov changed the title New osparc classifiers with scicrunch RRIDs (⚠️ devops) New classifiers with scicrunch RRIDs (⚠️ devops) Dec 17, 2020
@pcrespov pcrespov merged commit f72ba62 into ITISFoundation:master Dec 17, 2020
@pcrespov pcrespov deleted the is334/osparc-classifiers branch December 17, 2020 13:35
Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

Adding some concerns and suggestions for the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:webserver issue related to the webserver service idea Proposed new functionality that is still not implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants