Add Pf8 by eselimnl · Pull Request #659 · malariagen/malariagen-data-python

eselimnl · 2024-11-21T13:33:05Z

This PR adds a new Pf8 API class for accessing the Pf8 data resource.

Pf8(): The Pf8() class is built upon the Pf7() template, and should not disrupt the functionality of Pf7() (verified via test_pf7_integration.py).

Sanger S3 Configuration: Updates have been made in malariagen_data/util.py to support data access on the Sanger S3 storage. For privacy reasons, the bucket name is not included in the PR (such as in the tests/test_pf8_integration.py).

Variant/altlen Field Handling: We observed that the variant/altlen field contains a single value rather than one per ALT, despite the number of ALT being set to 6. The cause of this behavior is unclear, but it may relate to the comment “special case (for altlen, number depends on ALT)” in scikit-allel. Since it is possible to calculate this value from REF and ALT, we are not concerned about it now. Therefore, this field is excluded from data_vars after reading the Pf8 zarr file.

This PR was previously created from a fork, so comments made there will also be followed here.

PR ready Pf8() PR ready Pf8()

ahernank · 2024-11-26T08:13:32Z

Just FYI, tests are failing here as the github-actions service account doesn't have permissions for the pf8 bucket.
I've assigned the permissions now, so hopefully they should run smoothly now.

eselimnl · 2024-11-26T10:42:26Z

That's extremely helpful, thanks @ahernank

alimanfoo

Thanks @eselimnl, a few small suggestions.

Btw currently the way the S3 URLs are handled will mean that chained URLs like simplecache::s3://... won't work. Perhaps that is fine for now, up to you.

alimanfoo · 2024-11-27T10:36:05Z

malariagen_data/util.py

+        # Create an S3FileSystem with custom endpoint if specified.
+        kwargs["anon"] = kwargs.get("anon", True)  # Default to anonymous access.
+        kwargs["endpoint_url"] = "https://cog.sanger.ac.uk"
+        kwargs["config_kwargs"] = kwargs.get("config_kwargs", config)


Suggested change

kwargs["config_kwargs"] = kwargs.get("config_kwargs", config)

kwargs.setdefault("config_kwargs", config)

alimanfoo · 2024-11-27T10:36:57Z

malariagen_data/util.py

+        }
+
+        # Create an S3FileSystem with custom endpoint if specified.
+        kwargs["anon"] = kwargs.get("anon", True)  # Default to anonymous access.


Suggested change

kwargs["anon"] = kwargs.get("anon", True) # Default to anonymous access.

kwargs.setdefault("anon", True) # Default to anonymous access.

alimanfoo · 2024-11-27T10:37:56Z

malariagen_data/util.py

+
+        # Create an S3FileSystem with custom endpoint if specified.
+        kwargs["anon"] = kwargs.get("anon", True)  # Default to anonymous access.
+        kwargs["endpoint_url"] = "https://cog.sanger.ac.uk"


Suggested change

kwargs["endpoint_url"] = "https://cog.sanger.ac.uk"

kwargs.setdwfault("endpoint_url", "https://cog.sanger.ac.uk")

alimanfoo · 2024-11-27T10:41:46Z

malariagen_data/plasmodium.py

            data_vars = self._add_extended_data(root, inline_array, chunks, data_vars)
+            data_vars.pop(
+                "variant_altlen", None
+            )  # variant_altlen is under investigation in Pf8.zarr


Perhaps it would be possible to handle this by removing the variable from the Pf8 config file, rather than here? In which case no changes to tests for pf7 or pv4 would be needed?

Thanks, changed accordingly.

eselimnl · 2024-11-28T15:05:42Z

Thanks @eselimnl, a few small suggestions.

Btw currently the way the S3 URLs are handled will mean that chained URLs like simplecache::s3://... won't work. Perhaps that is fine for now, up to you.

Thanks @alimanfoo, I have incorporated your suggestions. As you expected, I receive botocore.exceptions.NoCredentialsError when I tried simplecache::s3://..., but I couldn't figure out why this is the case.

eselimnl · 2024-11-30T22:25:19Z

Sorry @alimanfoo, debugging the chained URL issue took a while, but simplecache::s3://... works fine now.

alimanfoo · 2024-12-02T18:06:57Z

Sorry @alimanfoo, debugging the chained URL issue took a while, but simplecache::s3://... works fine now.

My apologies, I should've explained a bit better how chained URLs are handled in fsspec, at least as I understand it. In looking at this I've also realised the way this is done for GCS could be clearer. I've taken the liberty to push a commit here which consistifies handling of storage options for both GCS and S3 including the the case where URLs are chained. Would you mind taking a look and testing to see if this works for your S3 bucket?

alimanfoo · 2024-12-02T18:08:08Z

Hold that thought, looks like I've broken some tests, investigating...

alimanfoo · 2024-12-02T18:13:22Z

Bug fixed, ready for testing with S3.

eselimnl · 2024-12-02T19:16:36Z

Many thanks for sorting this out, works perfectly fine with S3 in my testing environment.

alimanfoo · 2024-12-03T08:59:53Z

Many thanks for sorting this out, works perfectly fine with S3 in my testing environment.

Cool, thanks @eselimnl. Happy for this to be merged?

eselimnl · 2024-12-03T09:05:26Z

Yes, I am happy for this to be merged

alimanfoo · 2024-12-03T09:51:25Z

Thanks @eselimnl, merged! 🎉

Would you like us to make a release?

eselimnl · 2024-12-03T09:53:35Z

Exciting! Yes please!

ahernank · 2024-12-03T09:58:08Z

Just to note here that the S3 and GCS buckets for Pf8 are not currently "publicly" accessible (i.e. not public/not part of the authentication process).

eselimnl · 2024-12-03T11:22:57Z

I noticed an oversight in the testing environment: S3 access requires the s3fs library to be installed. While this works fine if done on the user side, I realize I could have included it in the package as well. Would you like me to create a new PR to address this?

eselimnl and others added 4 commits November 19, 2024 23:02

initial pf8 gcs

8d876b4

PR ready Pf8() PR ready Pf8()

file path change

3a87d9e

Merge branch 'master' into pf8-gcs

7bd258a

Pf8()

04fc2e3

eselimnl requested a review from alimanfoo November 21, 2024 13:33

eselimnl added 4 commits November 26, 2024 01:58

s3 access rewired via fsspec

5e03835

fixes the check for annotation file

76727db

removes extra path

07241a1

altlen commented in test units

5e3f0cf

missed fix for altlen

7ec51d9

eselimnl mentioned this pull request Nov 26, 2024

Pf8() #658

Closed

alimanfoo reviewed Nov 27, 2024

View reviewed changes

alimanfoo changed the title ~~Add Pf8()~~ Add Pf8 Nov 27, 2024

eselimnl added 3 commits November 28, 2024 01:12

fixes for PR feedbacks

1ceab5d

fix-2 for PR feedbacks

c361cad

fix for syntax error

2ae9201

chained urls into s3 fs

d20f8f0

make handling of chained URLs consistent between GCS and S3

773d00e

fix bug

cf401c3

alimanfoo approved these changes Dec 3, 2024

View reviewed changes

Merge branch 'master' into gcs-pf8

d6e39c3

alimanfoo merged commit 6cd964b into master Dec 3, 2024

alimanfoo deleted the gcs-pf8 branch December 3, 2024 09:49

alimanfoo added the BMGF-068808 Work supported by BMGF grant INV-068808 (MalariaGEN 2024-2027). label Dec 4, 2024

	kwargs["config_kwargs"] = kwargs.get("config_kwargs", config)
	kwargs.setdefault("config_kwargs", config)

	kwargs["anon"] = kwargs.get("anon", True) # Default to anonymous access.
	kwargs.setdefault("anon", True) # Default to anonymous access.

	kwargs["endpoint_url"] = "https://cog.sanger.ac.uk"
	kwargs.setdwfault("endpoint_url", "https://cog.sanger.ac.uk")

Conversation

eselimnl commented Nov 21, 2024 • edited by alimanfoo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahernank commented Nov 26, 2024

Uh oh!

eselimnl commented Nov 26, 2024

Uh oh!

alimanfoo left a comment

Choose a reason for hiding this comment

Uh oh!

alimanfoo Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

alimanfoo Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

alimanfoo Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

alimanfoo Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

eselimnl Nov 28, 2024

Choose a reason for hiding this comment

Uh oh!

eselimnl commented Nov 28, 2024

Uh oh!

eselimnl commented Nov 30, 2024

Uh oh!

alimanfoo commented Dec 2, 2024

Uh oh!

alimanfoo commented Dec 2, 2024

Uh oh!

alimanfoo commented Dec 2, 2024

Uh oh!

eselimnl commented Dec 2, 2024

Uh oh!

alimanfoo commented Dec 3, 2024

Uh oh!

eselimnl commented Dec 3, 2024

Uh oh!

alimanfoo commented Dec 3, 2024

Uh oh!

eselimnl commented Dec 3, 2024

Uh oh!

ahernank commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eselimnl commented Dec 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eselimnl commented Nov 21, 2024 •

edited by alimanfoo

Loading

ahernank commented Dec 3, 2024 •

edited

Loading