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

[GSProcessing] Fix to make graph_name optional again #1050

Merged
merged 2 commits into from
Oct 1, 2024
Merged

[GSProcessing] Fix to make graph_name optional again #1050

merged 2 commits into from
Oct 1, 2024

Conversation

thvasilo
Copy link
Contributor

@thvasilo thvasilo commented Sep 30, 2024 •
edited
Loading

Issue #, if available:

Description of changes:

  • GSProcessing makes providing a graph name optional. After [Enhancement] Add a strict naming rule for graph names #1044 we were checking the graph name before we filled in default values.
  • We now provide a default value before the name validity check.
  • Added some tests to check various graph naming cases.

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

@thvasilo thvasilo added the 0.4 label Sep 30, 2024
@thvasilo thvasilo added this to the 0.4 release milestone Sep 30, 2024
@thvasilo thvasilo self-assigned this Sep 30, 2024
@thvasilo thvasilo added the ready able to trigger the CI label Sep 30, 2024
jalencato
jalencato reviewed Sep 30, 2024
View reviewed changes
Copy link
Collaborator

@jalencato jalencato left a comment

Choose a reason for hiding this comment

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

Another overall comment, do we try to use the PR to trigger one SageMaker job?

graphstorm-processing/graphstorm_processing/distributed_executor.py Outdated Show resolved Hide resolved
jalencato
jalencato approved these changes Sep 30, 2024
View reviewed changes
Copy link
Collaborator

@jalencato jalencato left a comment

Choose a reason for hiding this comment

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

LGTM

zhjwy9343
zhjwy9343 approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@zhjwy9343 zhjwy9343 left a comment

Choose a reason for hiding this comment

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

LGTM

@thvasilo thvasilo merged commit 72139d6 into awslabs:main Oct 1, 2024
3 checks passed
@thvasilo thvasilo deleted the gsp-graph-name-fix branch October 1, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@classicsong classicsong classicsong left review comments

@jalencato jalencato jalencato approved these changes

@zhjwy9343 zhjwy9343 zhjwy9343 approved these changes

Assignees

@thvasilo thvasilo

Labels
0.4 ready able to trigger the CI
Projects
None yet
Milestone
0.4 release
Development

Successfully merging this pull request may close these issues.
4 participants
@thvasilo @classicsong @zhjwy9343 @jalencato