Skip to content

fix: allow session launcher parameters to be reset#434

Merged
olevski merged 6 commits intorelease-amaltheas-migrationfrom
reset-session-launcher-params
Oct 8, 2024
Merged

fix: allow session launcher parameters to be reset#434
olevski merged 6 commits intorelease-amaltheas-migrationfrom
reset-session-launcher-params

Conversation

@olevski
Copy link
Copy Markdown
Member

@olevski olevski commented Oct 1, 2024

Allows the API to accept None as input for args, command and the session launcher resource class ID so that they can be reset to their defaults in patch endpoints.

@olevski olevski requested a review from a team as a code owner October 1, 2024 22:36
@olevski
Copy link
Copy Markdown
Member Author

olevski commented Oct 1, 2024

@olevski olevski force-pushed the reset-session-launcher-params branch 3 times, most recently from 659480c to 4ccfc03 Compare October 1, 2024 22:44
@olevski olevski force-pushed the release-amaltheas-migration branch 2 times, most recently from be5272b to b1932ee Compare October 4, 2024 09:47
@olevski olevski force-pushed the reset-session-launcher-params branch from 4ccfc03 to 3a75082 Compare October 4, 2024 09:48
@olevski olevski force-pushed the release-amaltheas-migration branch from b1932ee to fd2e56c Compare October 4, 2024 11:30
@olevski olevski force-pushed the reset-session-launcher-params branch from 3a75082 to 3930efa Compare October 4, 2024 11:50
@olevski olevski force-pushed the release-amaltheas-migration branch from fd2e56c to df3bb88 Compare October 4, 2024 15:14
@olevski olevski force-pushed the reset-session-launcher-params branch from 3930efa to 4c4b1d7 Compare October 4, 2024 15:17
@olevski olevski force-pushed the reset-session-launcher-params branch from 4c4b1d7 to a8165cb Compare October 7, 2024 20:28
Allows the API to accept None as input for args, command and the session
launcher resource class ID so that they can be reset to their defaults
in patch endpoints.
@olevski olevski force-pushed the reset-session-launcher-params branch from a8165cb to 5bbbf85 Compare October 7, 2024 22:06
Copy link
Copy Markdown
Member

@Panaetius Panaetius left a comment

Choose a reason for hiding this comment

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

We had to solve the same issue in renku-python in the past, but we solved it with a specific object instance, not a type: https://github.com/SwissDataScienceCenter/renku-python/blob/develop/renku/domain_model/constant.py#L23

The benefit is that you can just use object id equality instead of isinstance checks, so it functions much more like regular None does than just a type. E.g. https://github.com/SwissDataScienceCenter/renku-python/blob/develop/renku/domain_model/constant.py#L23

I.e. there's only ever a single NO_VALUE object that contains nothing, except for the python object id, which is the same.

This is a common idiom for sentinel values, there is a PEP to have better support for sentinels but it's still a draft: https://peps.python.org/pep-0661/

The downside is that the object() approach doesn't have a nice repr, though not sure if we care about that.

You can use a class with a repr, but then it's important that you use the type as the value, not an instance, to have it behave like None does. This is discussed in a bit more detail in https://python-patterns.guide/python/sentinel-object/ and https://www.revsys.com/tidbits/sentinel-values-python/

In any case, we should use one of the more standard approaches.

@olevski
Copy link
Copy Markdown
Member Author

olevski commented Oct 8, 2024

@Panaetius ok I will switch to something like the example from renku python.

@olevski olevski requested a review from Panaetius October 8, 2024 09:29
@olevski olevski merged commit 51bdf26 into release-amaltheas-migration Oct 8, 2024
5 checks passed
@olevski olevski deleted the reset-session-launcher-params branch October 8, 2024 11:31
olevski added a commit that referenced this pull request Oct 8, 2024
Allows the API to accept None as input for args, command and the session
launcher resource class ID so that they can be reset to their defaults
in patch endpoints.
olevski added a commit that referenced this pull request Oct 22, 2024
Allows the API to accept None as input for args, command and the session
launcher resource class ID so that they can be reset to their defaults
in patch endpoints.
olevski added a commit that referenced this pull request Oct 28, 2024
Allows the API to accept None as input for args, command and the session
launcher resource class ID so that they can be reset to their defaults
in patch endpoints.
olevski added a commit that referenced this pull request Oct 30, 2024
Allows the API to accept None as input for args, command and the session
launcher resource class ID so that they can be reset to their defaults
in patch endpoints.
olevski added a commit that referenced this pull request Oct 31, 2024
Allows the API to accept None as input for args, command and the session
launcher resource class ID so that they can be reset to their defaults
in patch endpoints.
olevski added a commit that referenced this pull request Nov 1, 2024
Allows the API to accept None as input for args, command and the session
launcher resource class ID so that they can be reset to their defaults
in patch endpoints.
olevski added a commit that referenced this pull request Nov 4, 2024
Allows the API to accept None as input for args, command and the session
launcher resource class ID so that they can be reset to their defaults
in patch endpoints.
olevski added a commit that referenced this pull request Nov 6, 2024
Allows the API to accept None as input for args, command and the session
launcher resource class ID so that they can be reset to their defaults
in patch endpoints.
olevski added a commit that referenced this pull request Nov 8, 2024
Allows the API to accept None as input for args, command and the session
launcher resource class ID so that they can be reset to their defaults
in patch endpoints.
olevski added a commit that referenced this pull request Nov 11, 2024
Allows the API to accept None as input for args, command and the session
launcher resource class ID so that they can be reset to their defaults
in patch endpoints.
olevski added a commit that referenced this pull request Nov 11, 2024
Allows the API to accept None as input for args, command and the session
launcher resource class ID so that they can be reset to their defaults
in patch endpoints.
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