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 cli #32

Merged
merged 45 commits into from
May 7, 2024
Merged

Refactor cli #32

merged 45 commits into from
May 7, 2024

Conversation

kdp-cloud
Copy link
Collaborator

@kdp-cloud kdp-cloud commented Apr 22, 2024

It's certainly not perfect but I hope a good start...

  • Addition logging
  • Addition of configuration file
  • Addition of pydantic model with validation
  • Refactoring CLI

@kdp-cloud kdp-cloud self-assigned this Apr 22, 2024
@kdp-cloud kdp-cloud marked this pull request as ready for review April 22, 2024 14:08
@kdp-cloud kdp-cloud added the enhancement New feature or request label Apr 22, 2024
Copy link
Collaborator

@apriltuesday apriltuesday left a comment

Choose a reason for hiding this comment

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

This is great! I actually learned a lot just by going through this code so thanks 👍

Just for fun, I also tested the installation and it worked flawlessly:

(venv) april@boudica:~/projects/MARS/mars-cli$ mars-cli -d health-check
############# Welcome to the MARS CLI. #############
Running in Development environment
Checking the health of the target repositories.
Checking development instances.
Webin (https://wwwdev.ebi.ac.uk/ena/submit/webin/auth) is healthy.
ENA (https://wwwdev.ebi.ac.uk/ena/submit/webin-v2/) is healthy.
Biosamples (https://wwwdev.ebi.ac.uk/biosamples/samples/) is healthy.
(venv) april@boudica:~/projects/MARS/mars-cli$ ls ~/.mars
app.log  settings.ini
(venv) april@boudica:~/projects/MARS/mars-cli$

mars-cli/README.md Show resolved Hide resolved
mars-cli/README.md Outdated Show resolved Hide resolved
from mars_lib.target_repo import TargetRepository, TARGET_REPO_KEY


class IsaBase(BaseModel):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not an ISA or a Pydantic expert, but I'm wondering whether Pydantic gives us a way to extend the base ISA schemas to make "MARS-ISA-JSON" a reality. The idea here would be something like:

  • ISA Python API contains the Pydantic models that exactly implement ISA JSON schema (if we can generate Pydantic classes from JSON schema automatically, this is hopefully not much overhead for their team - this code probably gives a good start).
  • MARS extends those models with additional requirements - again I'm not an expert but something like this gist (from this SO answer). This way our schema stays up-to-date with ISA, but we maintain our own additions to the schema.
  • MARS can then validate using our additional requirements, and also use ISA-tools' own object model to take advantage of things like GraphQL queries for filtering and so on.

(BTW none of affects this PR in particular, I'm just thinking/fantasizing out loud.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I agree with you.
There are python packages that generate pydantic models from json.
And pydantic models can spit out json schema as well.
Curious about what the rest thinks about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

how can we coordinate to reuse objects and functionality from the isa-api rather than re-implementing?

Note: the latest code now supports adding 'Comments' to Assays thus allowing to indicate which Repository to dispatch to.

Copy link
Member

Choose a reason for hiding this comment

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

@proccaserra We would love to reuse the ISA API if it meets all our needs. But we were puzzled how we would for example validate, using ISA-tools, the loaded isa structure in a more stringent way than the ISA-JSON validator currently does? Because in the latest implementation we used, even an empty json was seen as a valid ISA JSON :/

kdp-cloud and others added 2 commits April 24, 2024 13:36
Co-authored-by: April Shen <[email protected]>
Copy link
Member

@bedroesb bedroesb left a comment

Choose a reason for hiding this comment

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

I've tested these changes and they worked for me! Again a step closer ;)

@bedroesb bedroesb merged commit 4107ff8 into main May 7, 2024
16 checks passed
@kdp-cloud kdp-cloud linked an issue May 7, 2024 that may be closed by this pull request
@kdp-cloud kdp-cloud mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Merged
4 participants