Improve Type Hints + Increase Type Checking + Added Makefile #18
Conversation
…ucture - Update pyproject.toml to use PEP 621 project metadata - Remove husky and lint-staged configurations - Migrate type hints to Python 3.12+ style (using `|` for unions) - Simplify type annotations across multiple files - Remove unused `types.py` module - Update Makefile to use new mypy configuration - Remove unnecessary type imports from typing module - Standardize function signatures with modern type hints
- Modify .pre-commit-config.yaml to use local mypy hook and uv-lock - Remove explicit mypy additional dependencies in pre-commit config - Add type stub packages to pyproject.toml dev dependencies - Remove auto-generated requirements.txt and requirements-dev.txt files - Update uv.lock to include new type stub packages
- Update pyproject.toml to set mypy Python version to 3.11 - Maintain compatibility with current project Python version configuration
|
types also became sqlmesh_types because of a name clash. I am thinking about making dagster_sqlmesh into a package using the .toml file though, so that we don't have to use relative imports, which I think is what caused that issue in the first place. EDIT - Just realised that my file name change here could cause a breaking change and doesn't follow the standard of library_name.types, so let me try and think of a way to get back to that without causing a circular import issue due to the name clash (might just be that I accidentally modified something without realising it the first time). EDIT 2 - Ok, so no idea why I seemed to think that caused an issue originally (might have just been something silly at the time). have no changed the file name back to types again like it was originally and there doesn't appear to be any issues |
- Update .pre-commit-config.yaml to use official mypy mirror and add type stub dependencies - Remove local mypy hook and replace with upstream configuration - Update Makefile to simplify Dagster development commands - Remove unused sqlmesh_types.py and workspace.yaml files - Migrate SQLMeshModelDep and SQLMeshMultiAssetOptions to types.py - Update import paths in dagster.py to reflect file reorganization
- Update pattern matching syntax for `StopPlanEvaluation` event - Remove unnecessary type ignore comment - Simplify event handling logic in DagsterSQLMeshEventHandler
- Add caching for faster mypy runs with `--cache-dir` - Ignore missing imports from third-party packages - Optimize additional dependencies for type checking - Add polars-related type dependencies - Use minimal dependencies for core packages - Improve type stub package selection
- Add `dagster-dev` command to run Dagster development server - Add `dagster-materialize` command to materialize all Dagster assets - Update README.md with new Make commands for easier Dagster project interaction - Enhance documentation to explain new Makefile targets
|
I have updated the README and Makefile slightly as well, to help with initial setup. I have only tested this on Windows, but I used a similar makefile on Mac before, so hopefully the path syntax stuff still works fine. Will try to test later today if I get a chance (would be good to see what a fresh install looks like anyway) |
- Remove redundant `sample-materialize` target from .PHONY list - Simplify Makefile target declarations
- Add `clean-dagster` target to remove Dagster temporary files - Update `dagster-dev` command to set `DAGSTER_HOME` and use workspace.yaml - Add `dev` alias for `dagster-dev` for easier command usage - Update .gitignore to exclude Dagster storage and database files - Update README.md with alternative `make dev` command - Enhance Makefile PHONY targets to include `clean-dagster`
- Add `can_subset=True` to `sqlmesh_assets` decorator for improved asset subsetting - Introduce `SQLMeshConcurrentResource` with advanced execution and logging capabilities - Update sample project definitions to use new `SQLMeshConcurrentResource` - Simplify Dagster project configuration by removing default storage configurations - Add logging and event tracking methods for better observability
- Simplify model dag retrieval and pruning logic in SQLMeshConcurrentResource - Remove redundant dag generation steps - Directly use `mesh.models_dag()` for model selection - Streamline model processing workflow
- Introduce `PlanAndRunOptions` to consolidate plan and run configuration - Update resource and controller methods to support unified options structure - Add shared options handling across plan and run operations - Improve asset key generation in translator and utils - Simplify model selection and execution logic - Update type definitions to support more flexible configuration
No worries! I do think we would like to see smaller PRs. This is hard to categorize and is a bit of a wholesale set of changes. I would prefer smaller PRs in general as that allows me to make decisions based generally related changes. So please try to keep everything related to a single goal. I see here at least a few different changes:
We didn't want to support both. I'd like to stick to pyright as it's what the rest of our toolchain does. Is there a strong reason for using mypy? Can we not fix within the context of pyright?
I don't mind switching from poetry to uv but can we split this into a separate PR?
That seems interesting enough but can we put this with the uv pr perhaps? |
|
Oh didn't respond to this:
Hrm, I've not used .pre-commit. That being said, I'm not against entirely but 1) I think this PR is a rather large set of changes and would like to see it split up and 2) I would prefer not to change this aspect of the repo at this time. Additionally, I openly despise yaml 😆. If somehow it has a different format, I'd be more open to it. Is there a reason you want to remove husky here? At least for this iteration of the repo that is in our care, I'd prefer to use the tools that we use so our own maintenance burden has a more narrow focus. I realize that isn't always ideal but that is the current preference. Once this is merged into the dagster community integrations repo any set of changes there would be fine! |
| # Python setup commands | ||
| install-python: | ||
| ifeq ($(UNAME),"Darwin") | ||
| brew install python@3.12 |
There was a problem hiding this comment.
I'm not sure this kind of thing is the responsibility of this repository. You should have python installed.
There was a problem hiding this comment.
Fair enough. Will remove.
| endif | ||
|
|
||
| check-uv: | ||
| @if [ "$(shell uname)" = "Darwin" ]; then \ |
There was a problem hiding this comment.
Hrm. This is another thing that should not be the responsibility of this repository. I'd likely get rid of this as well
| # Node.js setup commands | ||
| install-node: | ||
| ifeq ($(UNAME),"Darwin") | ||
| brew install node@18 |
There was a problem hiding this comment.
Same as with the python part of this.
There was a problem hiding this comment.
yep will remove
Yep, totally understand. The PR is definitely too large, but I mostly wanted to show you the general things I was going for, but now that you have seen most of them, I will split this up and keep to very small changes (and only necessary ones - apologies!) |
|
Closing this PR in favour of some smaller, less shi**y ones |
Oh I brought husky back as well (I didn't have a specific reason to get rid of it other than me being a noob again, but it seems to be working fine again for me now, so now it's back (in th new UV PR that is) |
There were some missing return types and whatnot that I noticed once I added some of the typical mypy checks in, and I think these serve to make things a bit more robust / easier to understand for other contributors, particularly when it comes to 'go to definition' (the DagsterSQLMeshController had a few typing issues with to_asset_outs etc...) and also just for knowing what things do at first sight a bit more easily.
I am quite new to open source and struggled a bit to deal with pyright specifically here and the way that the pre-commit hooks were set up, so I switched them to something I was more used to just to be able to get things to work whilst still maintaining some level of sense, however, this introduces extra change that isn't 100% necessary and that may not be desired.
Do we care about the option of mypy vs pyright, and do we care about using a .pre-commit file vs. husky? Please let me know, and if it is an issue, I can always spend some more time to bring them back, but just thought I would at least get some form of a PR up first before struggling too much.
I also switched from poetry to UV, but am happy to bring that back to poetry if desired (I saw uv.lock in another one of your projects, so I figured that this change might be less contentious, but if it's an issue, I am happy to go back to poetry and keep things in line with how they were originally as the onus is on me to try and keep things the same without making breaking changes)
Oh, and I added Makefile to help with some common commands, especially when it comes to getting things set up. This should make it a bit easier for someone using the repo and trying to get set up.
Let me know your initial thoughts, and let me know what changes I need to make that are essential so that I can fix them, get this PR merged, and then hopefully begin working on getting the initial draft for dagster community contribution which I will make some effort to keep up to date with changes from this repo.
EDIT - I have also opted to keep some of the javascript / npm artifacts until I know what is happening with pyright (in the case I need to bring it back, I figured it's easier to leave them here for now)