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

Feature/private submission #21

Open
wants to merge 20 commits into
base: dev
Choose a base branch
from
Open

Feature/private submission #21

wants to merge 20 commits into from

Conversation

Vkale1
Copy link

@Vkale1 Vkale1 commented Oct 29, 2024

This is not to be merged. It is just a review for the first part change of the code so you don't get a massive batch of changes to review at once. This change solely tackles the API endpoints. Any comments on cleaner coding would be good. I will tackle the exceptions and tests in a later review.

Logic:
--private is an input parameter
Public - portal API search endpoints with no authentication.
Private:

  • Webin reports API. JSON if required metadata is within it otherwise XML.
  • All fields are renamed to match the fields that the public API returns.
  • Lat/Long formats are different.

@Vkale1 Vkale1 requested review from mberacochea and Ge94 October 29, 2024 10:01
Copy link
Member

@Ge94 Ge94 left a comment

Choose a reason for hiding this comment

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

Massive work for a ridiculous problem... Thank you lots V.
I think that as long as it works let's keep it like this until we know for certain that we need to reimplement it with the new webin-cli (unless we recycle this code for the assembly_uploader?). But for the future I'd suggest:

  • Reformat all URLs as constants (also the one below, in the genome submission part)
  • Write functions for post/get requests and data parsing/processing
  • This could include writing a set of constants for exceptions

But again... I'd leave this for now

genomeuploader/ena.py Outdated Show resolved Hide resolved
genomeuploader/ena.py Outdated Show resolved Hide resolved
genomeuploader/ena.py Outdated Show resolved Hide resolved
@Vkale1
Copy link
Author

Vkale1 commented Dec 27, 2024

New changes! @Ge94 @mberacochea

  • Separated calls to the private and public APIs
  • Add --private option. Try private first in this case and public afterwards
  • Reformatted to remove camelCase for variables and functions
  • Moved around functions which fit in the class to remove redundancy. Main function is simplified.
  • Reformatted so that genome uploader can be useable as a python library.
  • Removed command line credentials. Should be in a config, .env or exported new before. Credentials are handled in ena.py now. Added defaults and requirements to other arguments.
  • Unit tests for all ena calls and one exception test added.
  • Standard pre-commit formatting checks applied to all files.

The end-to-end test doesn't work locally. Seems to be fine on GitHub .. it's failing locally at the submission of the samples with a Forbidden error. I suspect it's a credential thing but I'm not sure how you got around this before?

@Vkale1 Vkale1 requested review from Ge94 and mberacochea and removed request for mberacochea December 27, 2024 16:36
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.

2 participants