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

Revise scalar units #36

Closed
wants to merge 2 commits into from
Closed

Revise scalar units #36

wants to merge 2 commits into from

Conversation

pmjordan
Copy link
Contributor

@pmbruun
Copy link

pmbruun commented May 15, 2024

https://github.com/oasis-tcs/tosca-specs/blob/e7f866dc5e3081b90c27bde9c81ce113061ea122/tosca_2_0/TOSCA-v2.0.md?plain=1#L4331C1-L4335C58

A TOSCA parser shall not attempt to convert a YAML !!int to a float. This requirement is necessary for avoiding rounding errors and ensuring portability. Users should thus add a “.0” suffix to literal integers that must be floats. Note that this even includes zero, i.e. users must specify “0” for a zero integer and “0.0” for a zero float.

In today's discussion we discovered that this rule will be awkward - even the proposed examples would fail if we always use float for scalar types:

   weight: 10 Kg
   height: 100 mm
   width: 125 mm
   throughput: 1 KiB

It would have to be

   weight: 10.0 Kg
   height: 100.0 mm
   width: 125.0 mm
   throughput: 1.0 KiB

I understand that in some contexts there are strong feelings against automatically converting 10 Kg to 10.0 Kg.

The first part of the argument is not really making a lot of sense: "This requirement is necessary for avoiding rounding errors..."

You can get rounding errors if converting from float to integer, but the other way around - yes, a float is not always a precise representation of an integral number, 10, but it would not become any more precise by writing 10.0.

We could chose between four options:

  1. "A TOSCA parser MUST convert a YAML !!int to a float" - I would vote for this
  2. "A TOSCA parser MAY convert a YAML !!int to a float" - This would solve the issue, but would snag on the second part of the argument "... ensuring portability"
  3. "A TOSCA parser SHALL NOT attempt to convert a YAML !!int to a float" - This is the current text, but we would not be allowed to write "10 Kg"
  4. "A TOSCA parser SHALL NOT attempt to convert a YAML !!int to a float except in scalar units" - this was proposed in the language meeting, but the exception is too irregular for my taste. In scalar units the parser either ...
    1. ... MUST convert - For portability
    2. MAY convert - Looser, but compromises portability

Three other notes:

  • Key words like "MUST" MUST be in all caps in our specification: https://github.com/oasis-tcs/tosca-specs/blob/working/tosca_2_0/TOSCA-v2.0.md#key-words
  • The ISO prefix for multiplier 1000 "kilo" is k, not K. Writing 10 Kg looks very wrong - it should be 10 kg
  • I can see that you are using TAB-characters in the YAML examples - this is not valid yaml and creates a problem if someone tries to copy-paste text from our specification into their own YAML files. Also you cannot use the '''yaml (where ' should be backquote) annotation to get proper highlighting of YAML text

@pmbruun pmbruun mentioned this pull request May 15, 2024
Open
@pmjordan
Copy link
Contributor Author

pmjordan commented May 15, 2024 via email

@tliron
Copy link
Contributor

tliron commented May 15, 2024

I see no problem in automatically converting a YAML literal integer to a float if the scalar's type is float. Actually, more specifically: I see no value in emitting an "incorrect type" error in this case. Forcing the addition ".0" to every integer does nothing except emphasize to human readers that the value is a float. But isn't that obvious to readers?

The internal data storage is of the correct type (float or int) and that's what matters and should be maintained. We should definitely support both types of scalars.

For TOSCA 3.0 maybe we can also support imaginary numbers. :)

@pmbruun
Copy link

pmbruun commented May 15, 2024

@pmjordan Thanks

Are there specific places where my text needs revision on this point?

Probably not your text, but my initial quote, didn't use all caps:

A TOSCA parser shall not attempt to convert ...

Should be

A TOSCA parser SHALL NOT attempt to convert ...

I've done this throughput the document. Both Github and Visual Studio Code seem to highlight as YAML correctly.

The tabs in the pull-request look like this in GitHub:
image

@pmbruun
Copy link

pmbruun commented May 15, 2024

@tliron It looks like we all agree.

Only thing is this:

We should definitely support both types of scalars.

I am with you, but in today's meeting it was concluded that float should work for all scalars, although 0.3 B makes little sense.

I believe the reason was that having both integer and float added syntactic complexity to specification of new scalar types. Is that correctly understood?

@tliron
Copy link
Contributor

tliron commented May 15, 2024

I believe the reason was that having both integer and float added syntactic complexity to specification of new scalar types. Is that correctly understood?

I disagree with that decision, then. Yes, there's syntactic complexity, but I think it's necessary. That's exactly the kind of loss of precision we cannot allow in storage. If we support scalars, we should do it properly. (Honestly, I would prefer support for unsigned ints, too, but these are not supported by YAML.)

Can't we use a data type derived_from keyname to derive from either integer or float?

@pmjordan
Copy link
Contributor Author

Regarding integers and floats when used with scalars:
Yes @tliron , I did originally propose that scalars could derive from float or integer. In the meeting it was argued that there was no practial scalar that would be limited only to integers. I accepted that thus scalars could always be float. We agree that there is no sense in throwing an error if a scalar is assisgned an integer value, instead the orchestrator should convert to a float. However there still seem to be a debate regarding whether this automatic type conversion should be limited to scalars or should apply throughout the TOSCA syntax. In my view if it applies throughout TOSCA then there is little point having the integer type. I would prefer it was limited to scalars.

Regarding yaml rendering
@pmbruun I think I misunderstood. I accept that I should not use tabs. I thought you objected to '''yaml in general.

@tliron
Copy link
Contributor

tliron commented May 16, 2024

I think of this in the opposite direction -- we absolutely want to distinguish floats and integers for storage and transfer purposes in the world of node representations. It's the lack of this distinction that makes JSON such a pain in many environments. And it's not just about literal values written in YAML: remember that properties and attributes are typed data placeholders and the actual data can come from user inputs and from the environment. Why introduce the possibility for subtle, painful rounding errors and overflows? (Which in some cases can be understood as security bugs.) And if we have that distinction, then scalars should follow.

(Actually, I would say that we can even introduce things like precision and endianness to number representations. But since YAML doesn't handle that, it could be handled with metadata for users and orchestrator that need it.)

I hope we can reopen and revisit this. TOSCA is a very rich data modeling language for data types, I would think it should evolve over time and not remove important features. We had integer scalars (implicitly) in TOSCA 1.3, is it really so hard for us to come up with syntax to allow it in 2.0?

@lauwers
Copy link
Contributor

lauwers commented May 16, 2024

We have two separate issues, I believe:

  • If we assign a YAML integer value to a TOSCA property of type float, should the processor automatically convert? My vote is YES!
  • should we support both integer and float scalar units. I'm OK with both. We could introduce an extra keyname (sort of like entry_schema in lists or maps) that specifies which one is used.

@pmjordan pmjordan mentioned this pull request May 16, 2024
Merged
@pmjordan
Copy link
Contributor Author

pmjordan commented May 16, 2024 •
edited
Loading

I've created pull #42 to implement the solution based on the current working branch, with support for both integer and float in scalar units and requiring parsers not to error when assigning a YAML int to a TOSCA property of type float. Sorry I messed up when naming that pull request.

lauwers pushed a commit that referenced this pull request May 16, 2024
@pmjordan
unit table in line, type conversion allowed (#42)
72fb86d 
A new pull request to replace #36
@pmjordan pmjordan closed this Jul 4, 2024
@pmjordan pmjordan deleted the scalar-unit branch July 4, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pmjordan @pmbruun @tliron @lauwers