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

Parallel read option for GCR #621

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

patricialarsen
Copy link

@patricialarsen patricialarsen commented Feb 17, 2023

Adding parallel read using the config overwrite functionality and normal GCR.

An easy way to test this:
On a jupyter environment open up a terminal and run

source /global/common/software/lsst/common/miniconda/setup_current_python.sh

and then test using
mpirun -np 1 python gcr_test.py
with different numbers of processes, where the test code is

import sys
sys.path.insert(0,path_to_gcrcatalogs)
import GCRCatalogs
from mpi4py import MPI
import numpy as np
comm = MPI.COMM_WORLD
size = comm.Get_size()
rank = comm.Get_rank()

catalog = 'cosmoDC2_v1.1.4_small'
quantities=['ra']

rank_cat = GCRCatalogs.load_catalog(catalog, config_overwrite={'mpi_rank': rank, 'mpi_size': size})
data_rank = rank_cat.get_quantities(quantities)

print("on rank ", rank," of ", size, " we have ", len(data_rank['ra']), " galaxies in ", catalog )

def send_to_master(value):
    count = len(value)
    tot_num = comm.reduce(count)
    counts = comm.allgather(count)
    if rank==0:
        recvbuf = np.zeros(tot_num).astype(np.float64)
    else:
        recvbuf = None
    displs = np.array([sum(counts[:p]) for p in range(size)])
    comm.Gatherv([value,MPI.DOUBLE], [recvbuf,counts,displs,MPI.DOUBLE],root=0)
    return recvbuf

data_master={}
for quantity in quantities:
    data_master[quantity] = send_to_master(data_rank[quantity])

if rank==0:
    print("master rank has ", len(data_master['ra']), " galaxies in ", catalog )

@patricialarsen
Copy link
Author

patricialarsen commented Feb 17, 2023

There are a few changes I need to

  • firstly to fix the permissions on the files
  • secondly to merge updates from the main branch

Some things to note:

  • If you try to load a catalog in parallel which does not support the functionality, it gives a Runtime error.
  • The catalogs I've updated to include this are basically everything that allows native filters, with a handful of exceptions in which I don't adequately understand the underlying data structure or the reader enough to safely do this yet.

@yymao
Copy link
Member

yymao commented Feb 20, 2023

Thank you @patricialarsen! Sorry, I only had a bit of time to skim over it, and I'll take a closer look soon.

I do have a question about the permissions. Can you explain the issue a bit more. It's ok if we have to change the permissions, but I worried that people may not know this when creating new configs. I wonder if there's some alternatives that are more future-proof?

@patricialarsen
Copy link
Author

We don't need to change the permissions, it's just that for sprint-week events I've had people link to my local repository to access the reader and altered the permissions in doing so, so I need to reset these back to the default.

@patricialarsen
Copy link
Author

patricialarsen commented Feb 20, 2023

As a side note I believe we can make the permissions settings more general by using the core.fileMode and core.sharedRepository config settings, but am not entirely sure how these work. I believe setting the first of these to false stops git from tracking the permission changes in the repository should allow me to make local permissions changes without it causing these problems

@patricialarsen
Copy link
Author

You should also note that this pull request adds readers for the DP0.2 object catalogs

@yymao
Copy link
Member

yymao commented Mar 6, 2023

@patricialarsen thanks for updating the PR and sorry for the delay. Looking at the changes to the readers, I wonder if it's worthy creating a new base class, say BaseMPIGenericCatalog.

class BaseMPIGenericCatalog(BaseGenericCatalog):
    def __init__(self, **kwargs):
        self._rank = int(kwargs.pop('mpi_rank'))
        self._size = int(kwargs.pop('mpi_size'))
        super().__init__(**kwargs)

This way, it's more clear which readers support MPI (and in those cases, mpi_rank and mpi_size are required). For readers that doesn't support MPI (still uses BaseGenericCatalog), any additional kwargs are just ignored (which is more consistent with current behavior).

Thoughts?

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