Add flower script

[holgerroth]

Fixes # .

Description

Add a getting started script using Job API for Flower.
Update flower client_fn syntax.

Types of changes

• Non-breaking change (fix or new feature that would not break existing functionality).
• Breaking change (fix or new feature that would cause existing functionality to change).
• New tests added to cover the changes.
• Quick tests passed locally by running ./runtest.sh.
• In-line docstrings updated.
• Documentation updated.

[holgerroth]

/build

[YuanTingHsieh]

Mostly LGTM, please confirm if we need to ping a specific Flower version or not.

[YuanTingHsieh]

we might need to ping specific version here as Flower is still changing their api implementation?

[yanchengnv]

Yes, we do need to pin a specific version of flower.
I asked them for a fix version number but have not got response from them. Their main developer Pan is attending a conference. Not sure when he will be able to get back to us.

[chesterxgchen]

I talked to pan, he said already respond to you ( he message from LinkedIn, He is done with 6 hr training workshop). This week will be the final change on CLI

[chesterxgchen]

This syntax is strange, I don't think we should do this. should the client.py goes to the FlowerExecutor,

executor = FlowerExecutor(client_app ="client:app", task_scripts = "client.py")
job.to_clients(executor)

the syntax

Job.to(, ) , seems very strange to me, and in most cases, doesn't make sense. I didnt know until QA highlight this.

Not all executor has task executor file. Add special syntax, take any string path seem to be a big assumption.
And also different from the rest of syntax, job.to(x, ), where x is component.

    elif isinstance(obj, str):  # treat the str type object as external script
            if target not in self._deploy_map:
                raise ValueError(
                    f"{target} doesn't have a `Controller` or `Executor`. Deploy one first before adding external script!"
                )

            self._deploy_map[target].add_external_scripts([obj])

I strongly suggest disasble this syntax. @yanchengnv @YuanTingHsieh @yhwen @holgerroth what are you opinions on this ?

[holgerroth]

Adding @yhwen. Supporting placing of scripts to server/clients with to() is a general purpose solution. We have that built into the JobAPI to support any component that might refer to some code that needs to be deployed as part of the job. For example, now flower might also require a . toml file to be place at clients/server.

So, to me it makes sense and is an extensible solution for different use cases.

[chesterxgchen]

the Git Rendering wipe out the original string, what wrote was "job.to( left bracket "<" string path right bracket ">", site_name)
not comment to job.to() but specific take any string and add external path.

job.to(project.toml, site-1)
job.to(train.py, site-2)
job.to("my name", site-3)

all goes to an external directory feel strange to me.

[holgerroth]

we could do it so python modules are supported directly

import client
job.to_clients(client)

but then you still cannot add config files that might be needed by a component.

[chesterxgchen]

now, I am looking at .to_client() syntax, I also confused. which site_name this one add to ? it is suppose to be an syntax sugar wrapper. If the Client is Client model intend to have script executor, why the component doesnt include script or configure file as argument ?

[holgerroth]

it's to_clients(). Only use when sending the component to all clients.

[chesterxgchen]

ok, to_clients() make sense, but this is not address the original question though

[yanchengnv]

The job.to(thing, target) syntax is confusing since the 1st arg could be many things. Perhaps we could improve by defining multiple input args, one arg for each type of things, like this:

job.to(scripts=[list of scripts], dirs=[list of dirs], objects=[list of objs], targets=[list of targets])

It is possible or maybe even desirable to send multiple types of things in one call - you shouldn't have to call job.to() many times.

[chesterxgchen]

See my comment for discussion

[holgerroth]

Converted to draft as we work on an alternative way to deploy the depending scripts.

[yanchengnv]

Please do not merge this PR. After job API refactoring, this will be rewritten in a better way.