argoproj-labs / hera

Hera makes Python code easy to orchestrate on Argo Workflows through native Python integrations. It lets you construct and submit your Workflows entirely in Python. ⭐️ Remember to star!
https://hera.rtfd.io
Apache License 2.0
562 stars 105 forks source link

Challenging to tell when to use `models` vs not. Can we unify the objects into a single init? #800

Open flaviuvadan opened 11 months ago

flaviuvadan commented 11 months ago

It is currently challenging to know when to import the models module and use objects from that with Hera. Some users might expect that import hera.workflows.* gives them everything they need. However, imagine using ttl_strategy, which is easily available on a container but the GC strategy import must come from models, which is not as obvious. We should consider unifying the imports somehow

elliotgunton commented 11 months ago

I had an idea - but I'd be wary of doing something not obviously backwards-compatible or that blocks us in future - which would be to import all the models into hera/workflows/__init__.py (to allow import hera.workflows.* as you mentioned (although that import statement itself seems like a bad idea...)), or make lightweight wrappers around the models. We might be able to then expand on what the Hera classes offer on top of the models.

Would it make sense to make a docs-only change which addresses https://github.com/argoproj-labs/hera/issues/774 first?

flaviuvadan commented 11 months ago

make lightweight wrappers around the models

Could we automatically do this?

flaviuvadan commented 11 months ago

make lightweight wrappers around the models

Could we automatically do this?

@elliotgunton @ljyanesm I imagine the algorithm is something like:

What do you think?

hermesribeiro commented 10 months ago

This can become less challenging if all models appear in the docs. A user can search for say, Affinity, and be redirected to the models page and infer the import path. Currently only hera.workflows.models.io.argoproj.workflow.v1alpha1 is included and others should be included as well. Maybe adding each module file in a separate URL like https://hera.readthedocs.io/en/latest/api/workflows/models/io/argoproj/workflow/v1alpha1 or something along these lines instead of adding them all into the models page can help making the section lighter.

However, other than searching elsewhere, there is absolutely no way to know semantically, syntactically or structurally which Argo yaml objects are hera workflows classes and which are only pydantic models beforehand, and even this assumes that the user knows that there is a difference at all. So a :heavy_plus_sign: 1 to unifying.

Should I open a separate issue for the models doc? I can open a PR too in the coming days too.

flaviuvadan commented 10 months ago

@hermesribeiro an update to the docs sounds really great as a starting point until we figure out exactly how to approach this!