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

Python Refactor / Improvements / Add Documentation #13

Merged
merged 11 commits into from
Feb 1, 2025

Conversation

japerry911
Copy link
Contributor

@japerry911 japerry911 commented Feb 1, 2025

What changes are being made and why?

Hello, I needed to update some of the Kestra Python library for our internal Streamlit app; I thought it would be worthwhile to open a PR for the Fork, in case there is interest in how it is updated.

Updates:

  • more thorough documentation
    • added README
    • added more docstrings and updated existing ones
  • added ability for Flow class to use API token and Tenant
    • tenant is passed through init method
    • API token is read from Environment variable, KESTRA_API_TOKEN
  • ran black generic formatting on Python entities
  • updated unit tests
  • added retries for some 4xx and 5xx errors for Flow class

I am sorry that this PR is not super thorough, that the unit tests are not expansive, and that the commits are not conventional standard (I literally just saw that requirement when I opened PR description 😿 🤦 .

I ended up doing it on the side so that we could have the API Token/Tenant functionality on our end. I am happy/definitely would be grateful to help upgrade and add/improve the Kestra Python Library more starting in the near future after this streamlit POC I am working on passes.

Would that be helpful?

Notes/Thoughts

  • Version sets as 0.0.0 when installing locally since I didn't set environment variable, just raising that if this is merged
  • Logging is not really working in execute as far as I can tell, if you all want me to help contribute on this, I can add in some better logging
  • I tried to keep everything the same (methods/class/files) to make it backwards compatible/mergeable - if you all want me to help on any of this, and want to discuss possible different architecture, I am happy to participate
  • I didn't really spend a great deal on testing due to some other prioritizations, but I can add more thorough testing in the future
  • I am not really knowledge of Timer/Counter methods so I sort of guessed on docstrings for those

How the changes have been QAed?

  • executed in our streamlit app:
def execute_and_poll_flow(
    flow_id: str,
    namespace: str,
    inputs: dict | None = None,
):
   """
  KESTRA_HOSTNAME is set
  KESTRA_API_TOKEN is set
   """

    result = Flow(
        wait_for_completion=True,
        poll_interval=10,
        labels_from_inputs=False,
        tenant="REDACTED",
    ).execute(
        flow=flow_id,
        namespace=namespace,
        inputs={
            "name": "Skylord",
        },  # type: ignore
    )

    return result
id: helloWorld
namespace: demo
description: |
  This flow logs a message to the console, and used for demoing and testing purposes.

inputs:
  - id: name
    type: STRING
    required: true

tasks:
  - id: hello
    type: io.kestra.plugin.core.log.Log
    message: "Hello {{ inputs.name }}"

Screenshot 2025-01-31 at 11 47 02 PM

(Skylord is a 🐶)


Setup Instructions

Let me know any and all updates needed, happy to help!

Copy link
Member

@anna-geller anna-geller left a comment

Choose a reason for hiding this comment

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

thanks a lot, Jack 🙏

python/README.md Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
python/README.md Outdated Show resolved Hide resolved
@japerry911
Copy link
Contributor Author

Thank you for review @anna-geller , I am working on some updates - hold off on merging please 🙏

@japerry911
Copy link
Contributor Author

japerry911 commented Feb 1, 2025

Changes Info

New

  • more thorough documentation
    • added README
      • remove "Copy README" step from GitHub Action publish in favor of Python-specific README.md
    • added more docstrings and updated existing ones
  • added ability for Flow class to use API token and Tenant
    • tenant is passed through init method
    • API token is read from Environment variable, KESTRA_API_TOKEN
  • add dev extras dependencies
    • black
    • flake8
    • isort

Improvements

  • ran black generic formatting on Python entities
  • updated unit tests
  • added retries for some 4xx and 5xx errors for Flow class
  • general refactor of the kestra.py file
  • finish polling for CANCELLED workflows
  • implemented @anna-geller PR feedback (thank you 🙏 )

Notes to be read prior to merge

  • I don't know the release process currently, but the Version is set by Environment Variable I believe, so when I install my branch it defaults to 0.0.0. Just raising that so it does not release as 0.0.0.
  • also unsure why - Main / Publish to Npm (pull_request) Failing after 6s - failed
  • I removed the copy README.md step from PyPi release in favor of using the new Python-specific README.md that I added. I am not sure if I did this correctly, unable to test this deployment portion.
  • I added some formatters/linter, but did not configure much really to start, just basic configurations - I figured these could evolve overtime, or it could move to ruff tool or something
  • I kept as setup.py, but in future, would be interested in potentially building Package with uv
  • This should be backwards compatible as far as I can tell.
  • created issue Python Refactor / Improvements (Tenant & API Token Flow params) / Add Documentation #14, and attached this PR to it 🚀

Thank you team for allowing me to contribute 😸 !

@anna-geller anna-geller merged commit f146dd3 into kestra-io:main Feb 1, 2025
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Python Refactor / Improvements (Tenant & API Token Flow params) / Add Documentation
2 participants