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

add pyright and e2e tests for CI/CD #53

Merged
merged 8 commits into from
Dec 9, 2024
Merged

add pyright and e2e tests for CI/CD #53

merged 8 commits into from
Dec 9, 2024

Conversation

C-Loftus
Copy link
Member

@C-Loftus C-Loftus commented Nov 28, 2024

  • Add pyright for type checking
    • Dagster is complicated so if we can preemptively prevent runtime errors by making sure the types are right in CI, I think that is worthwhile
  • Begin to add pytest tests for materializing assets
    • Only tests materializing the configs since it is complicated and unclear how to test dynamically partitioned assets
    • But it is a good start, is a sanity check that nothing major broke, and all the code is loaded in without errors before materializing
    • Can run these with python main.py test
  • Renamed code/ to userCode/ since there is already a code module that interfered with ours

@C-Loftus C-Loftus requested a review from webb-ben November 28, 2024 22:56
@C-Loftus
Copy link
Member Author

@webb-ben In addition to the pyright / e2e changes I also just fixed the concurrency issue. The issue was that when a parallel job tries to recreate the config, it will fail if it is in use in another container. So I just catch the exception and ignore it if that is the case. Its simple and not super elegant but definitely the easiest heuristic since we can assume we don't want to be regenerating configs in the middle of other crawls. Works fine for me with 3 parallel jobs.

Won't touch this branch any more for time being until approval. Should fix your issues.

image

@webb-ben webb-ben merged commit c1abbaf into main Dec 9, 2024
3 checks passed
@webb-ben webb-ben deleted the pyright branch December 9, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants