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

dashboard/master: Calling setattr_argument() for the same key but different type raises a TypeError on the master #2397

Open
systemofapwne opened this issue Apr 24, 2024 · 4 comments
Assignees

Comments

@systemofapwne
Copy link

Bug Report

One-Line Summary

Calling self.setattr_argument(key,...) for the same key but different type raises a TypeError on the master

Issue Details

Steps to Reproduce

  1. Load the minimal example experiment below via dashboard
  2. Click submit
class ArgError(EnvExperiment):
    def build(self):
        self.setattr_argument("detuning_blue", processor = NumberValue(default = 1, ndecimals = 2, scale=1.0, unit="Γ"))
        self.setattr_argument("detuning_blue", processor = Scannable(default=NoScan(1,1), scale=1.0, unit="Γ"))
    
    # NOTE: This minimal example experiment does not reflect our actual application. We rather see the issue, when
    # we import a sub-experiment in order to extend it for scanning:
    # def build(self):
    #   self.child_exp = ChildExp(self) # <-- Calls self.setattr_argument("detuning_blue", processor = NumberValue(default = 1, ndecimals = 2, scale=1.0, unit="Γ"))
    #   
    #   # Adding scannable here
    #   self.setattr_argument("detuning_blue", processor = Scannable(default=NoScan(1,1), scale=1.0, unit="Γ"))
    
    def run(self):
        pass

Expected Behavior

The second call to self.setattr_argument(key,...) should internally update the type of the argument.

Actual (undesired) Behavior

The master raises a TypeError

# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]: ERROR:worker(14925,arg_error.py):root:Terminating with exception (TypeError: float() argument must be a string or a real number, not 'dict')

# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]: Traceback (most recent call last): 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]:   File "/nix/store/kdba2hsq9ssa21n96zr6v3za09sk2hik-python3-3.10.11-env/lib/python3.10/site-packages/artiq/master/worker_impl.py", line 331,
# in main 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]:     exp_inst = exp((device_mgr, dataset_mgr, argument_mgr, {})) 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]:   File "/nix/store/kdba2hsq9ssa21n96zr6v3za09sk2hik-python3-3.10.11-env/lib/python3.10/site-packages/artiq/language/environment.py", line 251
# , in __init__ 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]:     self.build(*args, **kwargs) 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]:   File "/home/gardener/artiq/experiments/arg_error.py", line 6, in build 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]:     self.setattr_argument("detuning_blue", processor = NumberValue(default = 1, ndecimals = 2, scale=1.0, unit=" ~S")) 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]:   File "/nix/store/kdba2hsq9ssa21n96zr6v3za09sk2hik-python3-3.10.11-env/lib/python3.10/site-packages/artiq/language/environment.py", line 318
# , in setattr_argument 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]:     setattr(self, key, self.get_argument(key, processor, group, tooltip)) 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]:   File "/nix/store/kdba2hsq9ssa21n96zr6v3za09sk2hik-python3-3.10.11-env/lib/python3.10/site-packages/artiq/language/environment.py", line 311
# , in get_argument 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]:     return self.__argument_mgr.get(key, processor, group, tooltip) 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]:   File "/nix/store/kdba2hsq9ssa21n96zr6v3za09sk2hik-python3-3.10.11-env/lib/python3.10/site-packages/artiq/language/environment.py", line 220
# , in get 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]:     r = processor.process(self.unprocessed_arguments[key]) 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]:   File "/nix/store/kdba2hsq9ssa21n96zr6v3za09sk2hik-python3-3.10.11-env/lib/python3.10/site-packages/artiq/language/environment.py", line 183
# , in process 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]:     return float(x) 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]: TypeError: float() argument must be a string or a real number, not 'dict' 
# Apr 24 16:30:23 onion launch-artiq-master-no-git[1121378]: ERROR:master:artiq.master.scheduler:got worker exception in prepare stage, deleting RID 14925 

Your System

  • Operating System: Arch Linux
  • ARTIQ version: ARTIQ v8.8396+115415d.beta

Additional notes

The experiment works as expected when ran via artiq_run.

@dnadlinger
Copy link
Collaborator

dnadlinger commented Apr 24, 2024

The second call to self.setattr_argument(key,...) should internally update the type of the argument.

I think as a user I'd prefer getting a clear error message on the experiment side (exception thrown with better error message). Otherwise, doesn't this risk introducing unexpected dependencies on the order classes are instantiated?

@systemofapwne
Copy link
Author

[...] Otherwise, doesn't this risk introducing unexpected dependencies on the order classes are instantiated?

It depends on, what you want to do.

In our specific case, we have sub-experiments structure like this here

# This is a "Controler" class, that controls various aspects of an experiment. Such as turning one specific part on or off. Here, it sets a detuning
class Controler(HasEnvironment):
    def build(self):
        self.setattr_argument("detuning_blue", processor = NumberValue(default = 1, ndecimals = 2, scale=1.0, unit="Γ"))
    
    def init(self):
        # Will use self.detuning_blue attribute in here
        self.set_detuning(self.detuning_blue)
    
    def set_detuning(self, detuning):
        # Does not use self.detuning_blue in here but uses the argument passed

# The main Experimentm that works fine
class MainExperiment(EnvExperiment):
    def build(self):
        self.setattr_argument("some_other_attribute", processor = NumberValue(default = 1, ndecimals = 2, scale=1.0))
        self.Controler = Controler(self) # <-- This will call setattr_argument() on MainExperiment instance for arguments: detuning_blue
    
    def run(self):
        self.Controler.init()   # Initializes to default values. Not necessary but
        # Do other stuff below

# An experiment, that should replicated ALL parameters of MainExperiment and then specificly scan over one parameter
class ScannableExperiment(EnvExperiment):
    def build(self):
        self.setattr_device("scheduler")    # Used later in the run method below
        
        self.Exp = MainExperiment(self) # <-- This will call setattr_argument() on ScannableExperiment instance for arguments: detuning_blue, some_other_attribute
       
        # Now, we want to modify a specific attribute to make it scannable and still use its original name
        self.setattr_argument("detuning_blue", processor = Scannable(default=NoScan(1,1), scale=1.0, unit="Γ"))     # <- This line later causes an exception, since detuning_blue has been previously defined as "NumberValue" via the MainExperiment(self) call above and artiq thinks, it still is of that type, while the data of it is now a dictionary, according to the Scannable representation
    
    def run(self):
        # Loop over the scannable.
        # Pseudo code here: It decomposes the detuning_blue ScannableObject into individual "NumberValue" type 
        # and then loops over them. Individual experiments of type "MainExperiment" are then submitted to the scheduler, 
        # where the arguments of detuning_blue is scanned.

I have to admit, that this looks like a very special case. But it if the user specificically says "I overwrite this existing argument of type A to new type B", then the underlaying code should comply. Apparently, artiq seems not do to so.

For the time being, I have written an ugly workaround, that easily might fail in the future, if the implementation of the artiq environment changes:

class ArgReplicator(HasEnvironment):
    """ Replicates arguments of an experiment on a parent experiment """
    def __init__(self, exp):
        self._exp = exp
        self._args = {}
        # HACK: We disguise ourself to be the "argument manager" in order to intercept calls to self.__argument_mgr.get() inside artiq.environment.HasEnvironment.get_argument
        # NOTE: This could break in future Artiq versions, when HasEnvironment.__init__ is or the argument manager's get() method is getting refactored
        # dev_mgr =       exp._HasEnvironment__device_mgr
        # dataset_mgr =   exp._HasEnvironment__dataset_mgr
        # sched_def =     exp._HasEnvironment__scheduler_defaults
        super().__init__(exp)
        self._HasEnvironment__argument_mgr = self

    def get(self, key, *args):
        """ Intercept Artiq calls of artiq.environment.HasEnvironment.get_argument for later use """
        # NOTE: This could break in future Artiq versions, when artiq.environment.HasEnvironment.get_argument is getting rewritten
        self._args[key] = args
    
    def setattr_argument(self, key, processor=None, group=None, tooltip=None):
        """ Try to extract group and tooltip, if already was set from the experiment we derive from """
        if key in self._args:
            v = self._args[key]
            group = group or v[1]
            tooltip = tooltip or v[2]
        self._args[key] = (processor, group, tooltip)
    
    @property
    def args(self):
        """ Return a list of all arguments of the experiment """
        return list(self._args.keys())
    
    def __call__(self):
        if not self._exp: return
        for key, args in self._args.items():
            self._exp.setattr_argument(key, *args)

With this, we can now write our Scannable Experiment's build() method like this:

# An experiment, that should replicated ALL parameters of MainExperiment and then specificly scan over one parameter
class ScannableExperiment(EnvExperiment):
    def build(self):
        self.setattr_device("scheduler")    # Used later in the run method below
        
        repl = ArgReplicator(self)
        
        self.Exp = MainExperiment(repl) # <-- This will call setattr_argument() and internally the argument manager is intercepted by the ArgReplicator. Arguments are NEVER passed to the argument manager here
        
        repl.setattr_argument("detuning_blue", processor = Scannable(default=NoScan(1,1), scale=1.0, unit="Γ")) # Overwrites existing detuning_blue on the ArgReplicator object
        
        
        repl()  # Applies all arguments of the ArgReplicator on the instance of this very class here.
        

Since I am a fan of fixing problems rather than working around them, the argument manager should just allow changing previously defined arguments if the users choses so.

@SimonRenblad
Copy link
Contributor

SimonRenblad commented May 3, 2024

The experiment works as expected when ran via artiq_run.

I should note here (for the sake of completeness) that artiq_run only works if the "detuning_blue" argument is not set. For the minimal example above, the following code throws the same exception:

artiq_run <FILE> "detuning_blue={'value': 1, 'repetitions': 1, 'ty': 'NoScan'}"

Additionally, if the minimal example was altered slightly with StringValue instead of NumberValue:

class ArgError(EnvExperiment):
    def build(self):
        self.setattr_argument("detuning_blue", processor = StringValue(default = "string"))
        self.setattr_argument("detuning_blue", processor = Scannable(default=NoScan(1,1), scale=1.0, unit="Γ"))

No exception is thrown and the experiment is run without issues. (Calls to get_argument would not return the expected value however...)

@SimonRenblad
Copy link
Contributor

A few thoughts regarding this issue after tinkering with it for a bit.

The workaround provided works as long only as long as there is no logic in build that depends on get_argument since the processing of unprocessed arguments is deferred until the end of build. An example of this is shown below. I do not know how common such usage is, but we do expose get_argument so it should to be accounted for.

def build(self):
    if self.get_argument("checked", processor=BooleanValue(default=True)):
        # build logic
    else: 
        # other build logic

I also do not think that suppressing errors caused by processor.process() is a good solution, as it may lead to some confusing behavior and misleading error messages for easy mistakes.
Example: Entering an invalid string to a NumberValue would cause the message "Supplied argument(s) not queried in experiment".

One potential solution is to take the deferred concept from @systemofapwne's ArgReplicator and make it a part of the API. It may look something like this:

def build(self):
    self.defer_processing("detuning_blue")   # key is marked for deferral
    
    # these 2 calls no longer process the unprocessed arg, `self.detuning_blue` is set to default value
    self.setattr_argument("detuning_blue", processor = NumberValue(default = 1, ndecimals = 2, scale=1.0, unit="Γ"))
    self.setattr_argument("detuning_blue", processor = Scannable(default=NoScan(1,1), scale=1.0, unit="Γ"))
    
    self.set_deferred() # "detuning_blue" is processed by `Scannable` and set as attribute + kernel invariant

This would allow the usage of sub experiments as desired without having to change their interface, as defer_processing and set_deferred can be called by the top-level experiment. Obviously better names would be given for these methods as well...

Thoughts on this @systemofapwne?

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

No branches or pull requests

3 participants