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

[Docker] Build docker for Parmetis Partition #784

Merged
merged 30 commits into from
May 14, 2024

Conversation

jalencato
Copy link
Collaborator

@jalencato jalencato commented Mar 26, 2024

Issue #, if available:

Description of changes:

Successfully run on one and multiple instances:

Ways to build docker:

cd ~/graphstorm/docker
bash ./build_docker_parmetis.sh ~/graphstorm/ cyrus_test latest
docker run --network=host --memory="192g" --shm-size 192g -v ~/efs/mount/efs/:/efs -d --name test cyrus_test:latest
cd ~/graphstorm

To run parmetis partition:

python3 graphstorm/python/graphstorm/gpartition/dist_partition_graph.py --input-path /efs/ml-100k-gsp/ \
    --metadata-filename updated_row_counts_metadata.json \
    --output-path /tmp/local-part-test/ --num-parts 4 \
    --dgl-tool-path ~/dgl/tools/ \
    --ip-list ip_list.txt --partition-algorithm parmetis

As our previous discussion in the dev meeting, we decide to put the doc in another link. The command is here for reference about how to run the parmetis partition.

Update the test in the internal regression test pipeline.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jalencato jalencato marked this pull request as ready for review May 3, 2024 03:59
@jalencato jalencato added the ready able to trigger the CI label May 3, 2024
@jalencato jalencato requested a review from thvasilo May 3, 2024 19:02
Copy link
Contributor

@thvasilo thvasilo left a comment

Choose a reason for hiding this comment

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

A few comments added.

Also, we're missing SageMaker support? As is this will only run on EC2 clusters correct?

docker/build_docker_parmetis.sh Show resolved Hide resolved
docker/parmetis/Dockerfile.parmetis Outdated Show resolved Hide resolved
docker/parmetis/Dockerfile.parmetis Outdated Show resolved Hide resolved
docker/parmetis/Dockerfile.parmetis Outdated Show resolved Hide resolved
docker/parmetis/Dockerfile.parmetis Outdated Show resolved Hide resolved
python/graphstorm/gpartition/metis_partition.py Outdated Show resolved Hide resolved
python/graphstorm/gpartition/metis_partition.py Outdated Show resolved Hide resolved
@jalencato
Copy link
Collaborator Author

A few comments added.

Also, we're missing SageMaker support? As is this will only run on EC2 clusters correct?

SageMaker support is not in this PR. This PR is only for EC2 clusters(Batch). But eventually we will support SageMaker

@jalencato jalencato requested a review from thvasilo May 10, 2024 21:36
Copy link
Contributor

@thvasilo thvasilo left a comment

Choose a reason for hiding this comment

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

Can we take the chance and add some documentation?

Can add as an example here https://graphstorm.readthedocs.io/en/latest/scale/distributed.html#process-and-partition-a-graph

docker/build_docker_parmetis.sh Outdated Show resolved Hide resolved
docker/build_docker_parmetis.sh Outdated Show resolved Hide resolved
@jalencato
Copy link
Collaborator Author

Can we take the chance and add some documentation?

Can add as an example here https://graphstorm.readthedocs.io/en/latest/scale/distributed.html#process-and-partition-a-graph

As we previously discussed, we should add another separate part for all gpartition in the doc. What about let us put it in the next doc refactor PR. I suppose it should involve the docker building + gpartition process for parmetis/random + overview for the whole pipeline. (As the gpartition is closed to the gsprocessing, we should put them together to avoid to let users to be confused.)

And I just take another look at the doc mentioned above, the part there involves both simple processing and partition. If we are trying to put our partition part there, we should at least put the gsprocessing + gpartition command + overview about the distributed processing pipeline there. It will take a lot of space there, so I still suggest to keep our first idea, to put all the staff in the following new doc to make it more easy to understand for user.

@jalencato jalencato requested a review from thvasilo May 13, 2024 17:09
Copy link
Contributor

@thvasilo thvasilo left a comment

Choose a reason for hiding this comment

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

Let's add a task/issue to track the need for gpartition-specific docs, and parmetis-partition to our integration tests.

A couple of minor changes left, LGTM.

@jalencato
Copy link
Collaborator Author

Task for refactoring and adding doc: #833

@jalencato jalencato merged commit 3a9f7dd into awslabs:main May 14, 2024
5 of 6 checks passed
thvasilo added a commit to thvasilo/graphstorm that referenced this pull request May 17, 2024
*Issue #, if available:*

*Description of changes:*

Successfully run on one and multiple instances:

Ways to build docker: 
```
cd ~/graphstorm/docker
bash ./build_docker_parmetis.sh ~/graphstorm/ cyrus_test latest
docker run --network=host --memory="192g" --shm-size 192g -v ~/efs/mount/efs/:/efs -d --name test cyrus_test:latest
cd ~/graphstorm
```

To run parmetis partition:
```
python3 graphstorm/python/graphstorm/gpartition/dist_partition_graph.py --input-path /efs/ml-100k-gsp/ \
    --metadata-filename updated_row_counts_metadata.json \
    --output-path /tmp/local-part-test/ --num-parts 4 \
    --dgl-tool-path ~/dgl/tools/ \
    --ip-list ip_list.txt --partition-algorithm parmetis
```

As our previous discussion in the dev meeting, we decide to put the doc
in another link. The command is here for reference about how to run the
parmetis partition.

Update the test in the internal regression test pipeline.

By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice.

---------

Co-authored-by: runjie <[email protected]>
Co-authored-by: Theodore Vasiloudis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready able to trigger the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Add parmetis partition to the current partition pipeline
2 participants