Skip to content

Style edits#7

Merged
lamikr merged 6 commits into
mainfrom
style-edits
Nov 12, 2025
Merged

Style edits#7
lamikr merged 6 commits into
mainfrom
style-edits

Conversation

@yugang-amd
Copy link
Copy Markdown
Contributor

Motivation

Technical Details

Test Plan

Test Result

Submission Checklist

@lamikr
Copy link
Copy Markdown
Collaborator

lamikr commented Nov 12, 2025

Thanks, your style edits looks good. There are couple of small details that were in my original text that could now be addressed as well.

  1. Following restriction mentioned in one sentence does not exist anymore in the rockbuilder. There can now be multiple separate application list files. (*.apps)

Ie. this sentence can now be removed:

Currently, RockBuilder is hardcoded to use apps/core.apps to determine the application list, but in the future, this functionality could be extended to support multiple project lists.

or replaced with something like "apps/core.apps" is an example of application list configuration file that can be given as a parameter for the rockbuilder. In addition of core.apps, there are for example a following configuration files available to define a other sets of applications to build: apps/pytorch_27_amd.apps, pytorch_28_amd.apps, pytorch_29.apps, pytorch_nightly.apps, sglang.apps

  1. I may have used little inconsistently the words "Project List Configuration Files" and "Application Set" to refer to same thing as I have struggled to find an short descriptive name for these *.apps files. Their purpose is to define a list of applications that together define a some type of bigger family of apps related to each others. For example pytorch version 2.7 related apps, or pytorch nightly related apps or applications required by sglang. I am open for suggestion for better wording for this term :-)

@yugang-amd
Copy link
Copy Markdown
Contributor Author

yugang-amd commented Nov 12, 2025

Thanks, your style edits looks good. There are couple of small details that were in my original text that could now be addressed as well.

  1. Following restriction mentioned in one sentence does not exist anymore in the rockbuilder. There can now be multiple separate application list files. (*.apps)

Ie. this sentence can now be removed:

Currently, RockBuilder is hardcoded to use apps/core.apps to determine the application list, but in the future, this functionality could be extended to support multiple project lists.

or replaced with something like "apps/core.apps" is an example of application list configuration file that can be given as a parameter for the rockbuilder. In addition of core.apps, there are for example a following configuration files available to define a other sets of applications to build: apps/pytorch_27_amd.apps, pytorch_28_amd.apps, pytorch_29.apps, pytorch_nightly.apps, sglang.apps

  1. I may have used little inconsistently the words "Project List Configuration Files" and "Application Set" to refer to same thing as I have struggled to find an short descriptive name for these *.apps files. Their purpose is to define a list of applications that together define a some type of bigger family of apps related to each others. For example pytorch version 2.7 related apps, or pytorch nightly related apps or applications required by sglang. I am open for suggestion for better wording for this term :-)

Per discussion, we'll standardize .apps files as Application Set Configuration Files, and .cfg files as Application Configuration Files.

Copy link
Copy Markdown
Collaborator

@lamikr lamikr left a comment

Choose a reason for hiding this comment

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

Thank you very much for the cleanups, it looks now good for me.

@yugang-amd yugang-amd marked this pull request as ready for review November 12, 2025 22:54
@lamikr lamikr merged commit 25500b1 into main Nov 12, 2025
1 check passed
@lamikr lamikr deleted the style-edits branch November 12, 2025 22:56
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