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.

Sign up for GitHub

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

Jump to bottom

Metagenomics Exchange upload command and related bookkeeping code #350

Merged
merged 49 commits into from
Feb 29, 2024
Merged

Metagenomics Exchange upload command and related bookkeeping code #350

merged 49 commits into from
Feb 29, 2024

Conversation

mberacochea
Copy link
Member

No description provided.

mberacochea and others added 30 commits January 29, 2024 11:09
@mberacochea
adds incremental ebi search dumping for analysisjobs
0a8a761
@mberacochea
integrates indexable sample data into analyses ebi search dump
4d51fe3
@mberacochea
adds ebi search dump for studies/projects (plus some tweaks to analyses)
240d8f4
@SandyRogers @mberacochea
resolve migration merge conflicts
ed97ccb
@mberacochea
adds support for chunking of analysis-runs ebi search dump
ab829f4
@mberacochea
bugfixes on analysis-run dumper
fcb9300
@KateSakharova @mberacochea
Add metagenomics exchange support
6438c3b
@mberacochea
Added 2 tests for ME, population script WIP
64f4900
@KateSakharova @mberacochea
Move metagenomic exchange commands to separate Class. Tests fail in P…
7484116 
…OST request
@mberacochea
My revised version:
fee19bd 
- Refactored the cli wrapper
- Refactored the settings to be in line with the rest
- Modified the unit tests
@KateSakharova @mberacochea
Modified ME dj function to add new ME records, fixed tests for ME and…
e7d9b17 
… api
@KateSakharova @mberacochea
Move test for mock API to test_mgx_api.py
2e0b019
@KateSakharova @mberacochea
Add settings
c92c381
@mberacochea
change models and add patch and delete to ME
bdac39c
@KateSakharova @mberacochea
Add delete and patch commands with tests to ME API
65e64e0
@mberacochea
add population scripts and tests for dry-run
3c664e6
@KateSakharova @mberacochea
add population scripts and tests for dry-run
f082415
@KateSakharova @mberacochea
change api adress
bb1ce81
@KateSakharova @mberacochea
change api adress
0f309c4
@KateSakharova @mberacochea
WIP
83b6b3a
@mberacochea
Minor refactor of the MGX indexation process WIP
97e226e
@mberacochea
This is still a WIP - MGX
e229f5c
@mberacochea
Fixed the migrations for EBI Search and MGX.
a6e9eda 
The test tests/me/test_populate_metagenomics_exchange.py::TestMeAPI::test_removals_dry_mode it's working now.
@KateSakharova
Fix some ME tests
2f83224
@mberacochea
WIP - migration tidy up
5961f7d
@mberacochea
Merge branch 'feature/metagenomics_exchange' of github.com:EBI-Metage…
02d3943 
…nomics/emgapi into feature/metagenomics_exchange
@mberacochea
Adjust field names to new convention (last_ebi_search_indexed)
60d2a37
@KateSakharova
WIP testing
ad539e7
@KateSakharova
Tests for populate ME
986f222
@KateSakharova
rm dev from tests and leave only DEV API
916ce45
KateSakharova and others added 7 commits January 31, 2024 16:25
@KateSakharova
fix pipeline mock
948caa8
@KateSakharova
fixes
6a55c40
@KateSakharova
add mock to patch test
18d4641
@KateSakharova
some fixes after review
bfb1b54
@mberacochea
Rename EBI Seach last_indexed fields
7dbbb14
@mberacochea
Add an extra migrate step to rename last_indexed -> last_ebi_search_i…
4a8f3b4 
…ndexed

This is needed because RenameField doesn't allow you to change
the db_column name.

With this change it seems to work, and the data should be kept:

```sql
--
-- Alter field last_indexed on analysisjob
--
ALTER TABLE "ANALYSIS_JOB" RENAME COLUMN "LAST_INDEXED" TO "LAST_EBI_SEARCH_INDEXED";
--
-- Alter field last_indexed on study
--
ALTER TABLE "STUDY" RENAME COLUMN "LAST_INDEXED" TO "LAST_EBI_SEARCH_INDEXED";
--
-- Rename field last_indexed on analysisjob to last_ebi_search_indexed
--
--
-- Rename field last_indexed on study to last_ebi_search_indexed
--
```
@mberacochea
Merge pull request #325 from EBI-Metagenomics/feature/metagenomics_ex…
633b3f7 
…change

Feature/metagenomics exchange
MGS-sails
MGS-sails reviewed Feb 12, 2024
View reviewed changes
Copy link
Contributor

@MGS-sails MGS-sails left a comment

Choose a reason for hiding this comment

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

Hi @mberacochea
Thanks for doing this.
Functionally speaking it all looks fine to me.
I just left a few minor comments that could perhaps improve the code in the long term.
But I don't think they are blockers for releasing this.

emgapi/metagenomics_exchange.py Show resolved Hide resolved
emgapi/metagenomics_exchange.py Show resolved Hide resolved
emgapi/metagenomics_exchange.py Outdated
logging.warning(f"Get API request failed")
return analysis_registry_id , metadata_match

if response.ok:
Copy link
Contributor

@MGS-sails MGS-sails Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think reducing the amount of nesting, can make this code more easier to maintain in the future.
Specifically referring to Lines: 111-141

@MGS-sails MGS-sails marked this pull request as ready for review February 12, 2024 10:15
SandyRogers
SandyRogers approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@SandyRogers SandyRogers left a comment

Choose a reason for hiding this comment

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

Thanks folks. Nothing much to add from me, other than:

  1. we will need a change to the config repo to add the ME config for dev and prod.
  2. We now usually include an API version bump on develop so that the pip installing the master branch always installs the latest commit.

ci/configuration.yaml Show resolved Hide resolved
@mberacochea
Copy link
Member Author

Thanks folks. Nothing much to add from me, other than:

1. we will need a change to the config repo to add the ME config for dev and prod.

2. We now usually include an API version bump on `develop` so that the pip installing the `master` branch always installs the latest commit.

PR 👍
Bump 👍

@mberacochea
A few bug fixes for the metagenomics exchange command and api wrapper.
b2d60bd 
Add support for Assembly and Run based jobs (both).
Remove the public check, we only submit public data to the exchange.
Normalize argument names in the MGX wrapper.
Added some docstrings
@mberacochea
Fix typos in method args
e3e9cfb
@mberacochea
Typo sequence_accesion -> sequence_accession
afa3f18
@mberacochea
Send the sequence_accession, not a Run object
c3ba441
@mberacochea
Add a bit of logging when posting a record to MGX
4b145c0
@mberacochea
Capture the API error data response, if present in the response.
6235f03 
The MGX API returns a json with the error message, which is useful
for debugging.
@mberacochea
Mock the Metagenomics Exchange API
fe6d2b8
@mberacochea
Fix mgx_api check_analysis invocation typo
783f275
@mberacochea
Mark piece of code to revisit and improve
1b031df
@mberacochea
Merge pull request #351 from EBI-Metagenomics/bugfix/metagenomics-exc…
c98171c 
…hange-bug-fixes

Fixes for a few bugs I encountered while testing the command
@mberacochea mberacochea merged commit cf7d51a into master Feb 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MGS-sails MGS-sails MGS-sails approved these changes

@SandyRogers SandyRogers SandyRogers approved these changes

Assignees

@mberacochea mberacochea

@KateSakharova KateSakharova

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mberacochea @SandyRogers @MGS-sails @KateSakharova