Skip to content
This repository was archived by the owner on Feb 20, 2025. It is now read-only.

Feat: Overhaul config loading #1

Merged
merged 13 commits into from
Jan 18, 2025
Merged

Feat: Overhaul config loading #1

merged 13 commits into from
Jan 18, 2025

Conversation

mrkaye97
Copy link

@mrkaye97 mrkaye97 commented Jan 13, 2025

Copy of hatchet-dev#296

Overhauling the client config to use Pydantic to get rid of a lot of convoluted parsing + validation code.

Main things here:

  • Got rid of the config.yaml stuff since nobody seems to be using that and it makes the code paths significantly more complicated.
  • Strict typing for all of the fields with the same defaults + field validators in the cases where there's parsing logic like extracting something from the JWT
  • Got rid of the ConfigLoader, since now we can instantiate the class directly
  • Enabled mypy for the new config class now that types check out

One other important thing is I think we should remove all of the load_dotenv() stuff in favor of expecting that the person running the example has the correct env vars set, since this is more likely to be how the SDKs are used, and using load_dotenv creates race conditions between imports that rely on the env vars and when that line is run

Some logs:

hatchet-sdk-py3.13matt@Matts-MacBook-Pro hatchet-python-v2 % pr simple
[DEBUG] 🪓 -- 2025-01-13 14:37:37,120 - creating new event loop
[INFO]  🪓 -- 2025-01-13 14:37:37,121 - ------------------------------------------
[INFO]  🪓 -- 2025-01-13 14:37:37,121 - STARTING HATCHET...
[DEBUG] 🪓 -- 2025-01-13 14:37:37,121 - worker runtime starting on PID: 88540
[DEBUG] 🪓 -- 2025-01-13 14:37:37,129 - action listener starting on PID: 88557
[INFO]  🪓 -- 2025-01-13 14:37:37,133 - starting runner...
[DEBUG] 🪓 -- 2025-01-13 14:37:37,133 - starting action listener health check...
[DEBUG] 🪓 -- 2025-01-13 14:37:37,137 - '_test-worker' waiting for ['myworkflow:step1']
[DEBUG] 🪓 -- 2025-01-13 14:37:37,673 - starting action listener: _test-worker
[DEBUG] 🪓 -- 2025-01-13 14:37:38,320 - acquired action listener: 34db85d3-7830-4862-af3d-1ed51e3b35b3
[DEBUG] 🪓 -- 2025-01-13 14:37:38,321 - sending heartbeat

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@mrkaye97 mrkaye97 requested review from grutt and abelanger5 January 13, 2025 18:15
Copy link

@grutt grutt left a comment

Choose a reason for hiding this comment

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

Good stuff! One outstanding todo

@staticmethod
def _get_env_var(env_var: str, default: Optional[str] = None) -> str:
return os.environ.get(env_var, default)
if not token:
Copy link

Choose a reason for hiding this comment

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

we may not need tenant id any more since the jwt wont be provisioned for a different tenant... something for a future pr

def __hash__(self) -> int:
return hash(json.dumps(self.model_dump(), default=str))

## TODO: Fix host port overrides here
Copy link

Choose a reason for hiding this comment

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

address todo?

Copy link
Author

Choose a reason for hiding this comment

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

I was planning to leave this for now since it's related to that outstanding PR in the non-fork - happy to address this now though!

Copy link
Author

Choose a reason for hiding this comment

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

this is done 👍

@grutt
Copy link

grutt commented Jan 14, 2025

One other thought, it might be good to add a DEBUG config value so we're able to set via env var in addition to the client init.

@mrkaye97
Copy link
Author

One other thought, it might be good to add a DEBUG config value so we're able to set via env var in addition to the client init.

This seems reasonable to me - another option would be to make the default for that param be set to something like os.getenv("HATCHET_WORKER_DEBUG", "False") == "True" or something, but I like the idea of trying to put everything in the config

@mrkaye97 mrkaye97 requested a review from grutt January 14, 2025 18:55
@mrkaye97 mrkaye97 merged commit 056f247 into main Jan 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants