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

Use latest lindi, pynwb, and hdmf with aws_region #59

Merged
merged 25 commits into from
May 20, 2024
Merged

Conversation

rly
Copy link
Contributor

@rly rly commented May 17, 2024

I updated the environment file. Please remake your conda environments after merging.

I added a couple of read remote LINDI file test cases. I think the local lindi test cases still apply, though we should update those to point to the dandiset with the test NWB files that we want to use.

I'm pointing the environment to branches of HDMF and PyNWB that support the aws_region argument to NWBHDF5IO so that we can specify the AWS region in ROS3 tests.

Also we no longer need to say load_namespaces=True. That is now the default.

@rly
Copy link
Contributor Author

rly commented May 17, 2024

nwb_benchmarks run --bench NWBLindiFileReadRemoteReferenceFileSystemBenchmark
nwb_benchmarks run --bench NWBLindiFileCreateLocalReferenceFileSystemBenchmark
nwb_benchmarks run --bench LindiFileReadLocalReferenceFileSystemBenchmark
nwb_benchmarks run --bench NWBFileReadBenchmark
nwb_benchmarks run --bench DirectFileReadBenchmark

These ran successfully. I have not yet run a full suite of benchmarks yet though.

@rly rly marked this pull request as draft May 17, 2024 23:02
@rly rly changed the title Use latest lindi, pynwb, and hdmf Use latest lindi, pynwb, and hdmf with aws_region May 18, 2024
@rly rly marked this pull request as ready for review May 18, 2024 00:58
@rly
Copy link
Contributor Author

rly commented May 18, 2024

I defined remote benchmarks as such:

lindi_remote_rfs_parameter_cases = dict(
    EcephysTestCase=dict(
        s3_url=get_s3_url(
            is_staging=True,
            dandiset_id="213889",
            dandi_path="sub-IBL-ecephys/sub-IBL-ecephys_ses-3e7ae7c0_desc-18000000-frames-13653-by-384-chunking.lindi.json",
        ),
        object_name="ElectricalSeriesAp",
        slice_range=(slice(0, 30_000), slice(0, 384)),
    ),
    OphysTestCase=dict(
        s3_url=get_s3_url(
            is_staging=True,
            dandiset_id="213889",
            dandi_path="sub-R6_ses-20200206T210000_behavior+ophys/sub-R6_ses-20200206T210000_behavior+ophys.lindi.json",
        ),
        object_name="TwoPhotonSeries",
        slice_range=(slice(0, 3), slice(0, 796), slice(0, 512)),
    ),
    IcephysTestCase=dict(
        s3_url=get_s3_url(
            is_staging=True,
            dandiset_id="213889",
            dandi_path="sub-1214579789_ses-1214621812_icephys/sub-1214579789_ses-1214621812_icephys.lindi.json",
        ),
        object_name="data_00002_AD0",
        slice_range=(slice(0, 30_000),),
    ),

I thought that this would create three test cases. Instead, it creates 9 - the cross of all the parameters. But all of the unintended combinations will fail. I think this is an issue within BaseBenchmark. @CodyCBakerPhD is this behavior correct? and if so, how do I create three test cases instead of the cross of all these options?

@CodyCBakerPhD
Copy link
Collaborator

@CodyCBakerPhD is this behavior correct? and if so, how do I create three test cases instead of the cross of all these options?

Well, that's cool...

First time running that parameterized pattern and it's revealed that that is indeed the intended behavior of ASV: https://asv.readthedocs.io/en/v0.6.1/writing_benchmarks.html#parameterized-benchmarks

There's not an easy way I can think of at the moment to resolve this at the parameter_case or Basebenchmark level (attempts to use parameterized package cause issues in benchmark discovery phase)

Only way will be to break up the multilevel dict-of-dicts to separate benchmarks. I'll do that in my PR

@rly
Copy link
Contributor Author

rly commented May 20, 2024

This fix in lindi makes the lindi ophys test case work. NeurodataWithoutBorders/lindi#71

@CodyCBakerPhD CodyCBakerPhD mentioned this pull request May 20, 2024
@CodyCBakerPhD CodyCBakerPhD merged commit 16bb196 into main May 20, 2024
2 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the lindi_update branch May 20, 2024 17:30
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