covalent: Error when parsing electron positional arguments in workflows

Details When workflows are executing, there is a bug when parsing positional arguments for electrons with more than 2 arguments. For example, consider the following electron

import xgboost as xgb
import pandas as pd

@ct.electron
def train(model: xgb.XGBModel, X: pd.DataFrame, Y: targets):
     model.fit(X, Y)
     return trained_model

@ct.workflow
def workflow():
   trained_model = train(model, features, targets)

When this workflow is dispatched, the argument model gets interpreted as a pandas dataframe and an AttributeError is raised since a dataframe has not fit method. The error can be cirumvented when passing arguments to electron using kwargs i.e

@ct.workflow
def workflow():
   trained_model = train(model=model, X=features, Y=target)

About this issue

Original URL
State: closed
Created 2 years ago
Comments: 19 (18 by maintainers)

Most upvoted comments

I have to say @WingCode you did a pretty good job on finding, explaining, and documenting this issue! Feel free to start working on a PR to resolve this and make sure you put a [unitaryhack] label in the title of it.

So to resolve this issue we should give args the kwargs treatment by persisting the order of the elements, I believe.

Yeah, that is a viable solution. If possible let’s try to find a way to solve this without adding additional attributes to the transport graph (using an edge number maybe?, although I’m not sure if something like that already exists in networkx), but if it seems to get to convoluted then we can go the route of storing the order as well.

kessler-frost on Jun 10, 2022

@kessler-frost Thanks for getting back with a detailed explanation! With your code, I am able to reproduce the issue in my local. Let me dig into the code and figure what’s wrong 👍

WingCode on Jun 7, 2022

PS: @wjcunningham7 has the demo in which it did not work.

santoshkumarradha on Jul 7, 2022

@kessler-frost, @scottwn Reopening this again as seems like few Demo workflows we are writing is still affected by it with Covalent==0.110.2. Do not have a minimum working example as the one we tested in demo was a complicated ML flow and seems to be working with kwargs passed, but throws up error related to argument being different type when passed by args.

santoshkumarradha on Jul 7, 2022

Finally I have figured out the issue! When we solely use only args in ct.electrons, it relies on insertion order of edges into networkx.DiGraph to determine the order of the arguments passed into the function. The problem happens when we try to serialise the graph using nx.readwrite.node_link_data before sending it over the network using: https://github.com/AgnostiqHQ/covalent/blob/8889f4423c80499115bd43a77f5a17e0667f4bc3/covalent/_workflow/transport.py#L302 This method doesn’t preserve the insertion order of edges hence when we deserialise here, https://github.com/AgnostiqHQ/covalent/blob/8889f4423c80499115bd43a77f5a17e0667f4bc3/covalent/_workflow/transport.py#L348 it doesn’t preserve insertion order of the edges.

Thereby finding the dependencies for particular node doesn’t follow the insertion order and we are solely trusting on the order of the edges to build the args: https://github.com/AgnostiqHQ/covalent/blob/8889f4423c80499115bd43a77f5a17e0667f4bc3/covalent_dispatcher/_core/execution.py#L120-L127

Networkx graphs doesn’t guarantee insertion order for edges.

So to resolve this issue we should give args the kwargs treatment by persisting the order of the elements, I believe.

I would love to work on this issue. I am all ears to any other better ways of tackling this.

WingCode on Jun 10, 2022

@WingCode Thanks for your contributions on this issue. We will circle back to you on updates from our end.

Cc @santoshkumarradha @venkatBala @kessler-frost

Emmanuel289 on Jun 7, 2022