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.

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

save the repr of template variables in database #4864

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented May 9, 2022

These changes close #4862

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg and conda-environment.yml.
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.

@wxtim wxtim requested a review from oliver-sanders May 9, 2022 14:05
@wxtim wxtim self-assigned this May 9, 2022
@wxtim wxtim added the bug Something is wrong :( label May 9, 2022
@wxtim wxtim added this to the cylc-8.0rc4 milestone May 9, 2022
@oliver-sanders
Copy link
Member

@oliver-sanders - From comments at conference

I think we should be either storing the "raw" strings or the Python repr() of the parsed types.

Storing the type as a separate field won't work because types can be nested e.g. {'foo': [{1, 2}, True, 42, 'string']}.

#4862 (comment)

@wxtim wxtim marked this pull request as draft May 11, 2022 11:03
@wxtim wxtim force-pushed the save_the_type_of_template_variables_in_database branch from 20772a0 to 47ea104 Compare May 11, 2022 11:47
@wxtim wxtim changed the title save the type of template variables in database save the repr of template variables in database May 11, 2022
@wxtim wxtim marked this pull request as ready for review May 11, 2022 11:48
@wxtim wxtim force-pushed the save_the_type_of_template_variables_in_database branch from 47ea104 to ce77fa8 Compare May 11, 2022 11:49
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Need to test manually but looks good.

@wxtim wxtim requested review from oliver-sanders and MetRonnie May 11, 2022 14:09
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Looks good, tested as working 🎉

@wxtim wxtim force-pushed the save_the_type_of_template_variables_in_database branch from ff756a9 to 20772a0 Compare May 12, 2022 08:39
@wxtim wxtim marked this pull request as draft May 12, 2022 08:43
@wxtim
Copy link
Member Author

wxtim commented May 12, 2022

I've badly messed up branches - fixing before undrafting.

@wxtim wxtim force-pushed the save_the_type_of_template_variables_in_database branch from 20772a0 to a90dd45 Compare May 12, 2022 08:56
@wxtim wxtim marked this pull request as ready for review May 12, 2022 08:57
@MetRonnie
Copy link
Member

MetRonnie commented May 12, 2022

I think a changelog entry wouldn't go amiss. Also a short description of the fix in the PR body, for the benefit of us from the future?

@oliver-sanders
Copy link
Member

oliver-sanders commented May 12, 2022

FYI: For piece of mind I had a quick crack at checking that repr representations support everything that literal_eval can parse. Here's what I came up with, as expected it didn't find any examples upto 12 chars long.

Compute heavy, good to check but we don't need anything like this in the test battery.

from ast import literal_eval                                              
from itertools import combinations_with_replacement                       
                                                                          
                                                                          
def range2(*args):                                                        
    for number in range(*args):                                           
        print(f'# {number}')                                              
        yield number                                                      
                                                                          
                                                                          
def generate(length):                                                     
    """Generate random strings with "length" characters."""               
    for chars in combinations_with_replacement(                           
        '\'"-_+=()<>[]{},./a1',                                           
        length                                                            
    ):                                                                    
        yield ''.join(chars)                                              
                                                                          
                                                                          
def parse(string):                                                        
    """Return True if the string is a valid Python literal."""            
    try:                                                                  
        literal_eval(string)                                              
    except Exception:                                                     
        return False                                                      
    return True                                                           
                                                                          
                                                                          
def test(string):                                                         
    """Ensure repr/literal_eval can serialise/deserialise this value."""    
    value1 = literal_eval(string)    
    try:    
        value2 = literal_eval(repr(value1))    
    except Exception:    
        return False    
    return value1 == value2    
     
     
invalid_reprs = (    
    string    
    for length in range2(0, 20)    
    for string in generate(length)    
    if parse(string)    
    if not test(string)    
)    
     
for item in invalid_reprs:    
    print(item) 

Some of the things literal_eval accepts are interesting though e.g:

  • {},.1 (implicit tuple)
  • """""""" (convoluted string)
  • ((((())))) (empty tuple)

Worth being aware that these are valid template variables!

@wxtim
Copy link
Member Author

wxtim commented May 12, 2022

Can I get a final check @oliver-sanders and @MetRonnie

@oliver-sanders oliver-sanders merged commit 727796a into cylc:master May 12, 2022
@wxtim wxtim deleted the save_the_type_of_template_variables_in_database branch May 12, 2022 14:20
@hjoliver
Copy link
Member

FYI: For piece of mind I had a quick crack at checking that repr representations support everything that literal_eval can parse.

Nice, but isn't that exactly the purpose of repr (so it had better work!)?

@oliver-sanders
Copy link
Member

Nice, but isn't that exactly the purpose of repr

In theory, yes, in practice however, it is implemented on a type-by-type basis (e.g. TaskProxy(repr(TaskProxy())) will not work). So long as literal_eval supports the output of repr for all of the types supported by literal_eval we are good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

template vars all read from DB as strings
4 participants