Skip to content

Application versions#1362

Open
dapomeroy wants to merge 6 commits intoGoogleCloudPlatform:developfrom
dapomeroy:application_versions
Open

Application versions#1362
dapomeroy wants to merge 6 commits intoGoogleCloudPlatform:developfrom
dapomeroy:application_versions

Conversation

@dapomeroy
Copy link
Collaborator

@dapomeroy dapomeroy commented Dec 15, 2025

This pull request introduces object versions, which allows for 'when' conditions to be set using versions or version ranges. This feature allows Ramble to better support applications as they change and develop over time. Included in this PR is a new unified 'wrf' application that will replace 'wrfv3' and 'wrfv4', which remain in the repo for now.

See the workspace config and application developer guide documentation for more details on how to use versions.

@dapomeroy dapomeroy force-pushed the application_versions branch from 8fedfa8 to f93f7c3 Compare December 19, 2025 21:38
@rfbgo
Copy link
Collaborator

rfbgo commented Jan 8, 2026

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a versioning system for applications and other objects, which is a significant new feature. The implementation correctly uses the packaging library for version handling and consistently applies version string parsing across the codebase. New directives for version management have also been added.

However, I've identified a few critical issues in the new ObjectVersion class, including an incorrect copy method implementation and a potential NameError in the satisfies method. There are also a couple of medium-severity issues related to redundant code and incorrect attribute naming. These issues should be addressed to ensure the stability and correctness of the new versioning feature.

rfbgo
rfbgo previously approved these changes Jan 23, 2026
@dapomeroy dapomeroy force-pushed the application_versions branch 4 times, most recently from 28c1b32 to 00ccc56 Compare January 26, 2026 17:10
@dapomeroy dapomeroy force-pushed the application_versions branch 9 times, most recently from bee07e1 to d06fa73 Compare February 11, 2026 05:01
@dapomeroy dapomeroy marked this pull request as ready for review February 11, 2026 05:38
@dapomeroy dapomeroy force-pushed the application_versions branch from 1caafbb to 99051f1 Compare February 11, 2026 05:40
@dapomeroy
Copy link
Collaborator Author

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a comprehensive versioning system for applications and other objects within Ramble. It's a significant feature addition that touches many parts of the codebase, from configuration and language directives to command-line tools and testing. The core idea of using the packaging library for version handling is solid. The changes to support versioned object names (e.g., app@1.0) are consistently applied. Overall, this is a great enhancement. I've found a few bugs in the implementation details and a place for potential refactoring for better readability and performance. Please see my detailed comments.

@dapomeroy dapomeroy force-pushed the application_versions branch from 99051f1 to 1e425e6 Compare February 11, 2026 19:55
@dapomeroy dapomeroy changed the title Application versions (WIP) Application versions Feb 11, 2026
@dapomeroy dapomeroy force-pushed the application_versions branch 5 times, most recently from 4d5f868 to 52668d2 Compare February 12, 2026 05:32
@douglasjacobsen
Copy link
Collaborator

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a comprehensive versioning system for applications and other objects within Ramble. This is a significant feature that allows for more precise dependency management and configuration. The changes are extensive, touching core logic, the domain-specific language, and numerous application files to adopt this new versioning capability. A new ObjectVersion class, based on the robust packaging.version library, is at the core of this feature, which is a solid design choice. The implementation appears well-thought-out and consistent across the codebase. I have found one bug in the output formatting logic that could lead to incorrect display of versioned strings, for which I've provided a suggestion.

@dapomeroy dapomeroy force-pushed the application_versions branch from a1a3ba8 to 4a7993b Compare February 13, 2026 04:53
Copy link
Collaborator

@douglasjacobsen douglasjacobsen left a comment

Choose a reason for hiding this comment

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

Thanks! This is looking good! I have several requests and nits.

is used to set a version, and ``when`` conditions can be described using the following syntax:

* ``application_version@<version_number>`` Apply to only a specific version.
* ``application_version@:<version_number>`` Apply to a range up to the specified version.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Up to an including?


* ``application_version@<version_number>`` Apply to only a specific version.
* ``application_version@:<version_number>`` Apply to a range up to the specified version.
* ``application_version@<version_number>:`` Apply to a range from the specified version.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Including again?

* ``application_version@<version_number>`` Apply to only a specific version.
* ``application_version@:<version_number>`` Apply to a range up to the specified version.
* ``application_version@<version_number>:`` Apply to a range from the specified version.
* ``application_version@<start_number>:<end_number>`` Apply to a range of versions.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Specify inclusion if possible.

* ``application_version@<version_number>:`` Apply to a range from the specified version.
* ``application_version@<start_number>:<end_number>`` Apply to a range of versions.

Version numbers must conform to `Python packaging.version`_ format. In some cases, it may be
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably talk about this. I think we originally wanted the front-end to support spack syntax and then we would convert it to python_packaging.version internally.

I haven't gotten through the rest of the PR yet, but without utilities to go back and forth, it seems like it could easily make aspects of workspaces confusing for users.

And it feels like we can make this easier internally too.

"""Print all objects with their latest versions."""
objs = [ramble.repository.get(name, object_type=object_type) for name in obj_names]
objs = [
ramble.repository.get(name.partition("@")[0], object_type=object_type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit; It might be nice if the partition calls could be added into the repository class, just so people don't have to remember to call it with partition.

return results.getvalue()


def sort_when(when: str):
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit; This might be better called "when_order", since it doesn't actually sort the when conditions.

if not search_description:
return False
obj = ramble.repository.get(obj_name, object_type=obj_type)
obj = ramble.repository.get(obj_name.partition("@")[0], object_type=obj_type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit; Same partition comment.

obj_name = obj
try:
obj_inst = ramble.repository.get(obj_name, object_type=obj_type)
obj_inst = ramble.repository.get(obj_name.partition("@")[0], object_type=obj_type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit; Same partition comment.


# Setup the application instance
app_inst = ramble.repository.get(final_app_name)
app_inst = ramble.repository.get(final_app_name.partition("@")[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit; Same partition comment


properties["config"]["enable_workspace_prompt"] = {"type": "boolean", "default": False}

properties["config"]["enable_strict_versions"] = {"type": "boolean", "default": True}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see it in this PR, but would it be useful to have a CLI flag for this?

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.

3 participants