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.

Sign up for GitHub

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

Jump to bottom

add pydantic only for apache-airflow 2.6 #725

Closed
wants to merge 9 commits into from
Closed

add pydantic only for apache-airflow 2.6 #725

wants to merge 9 commits into from

Conversation

pixie79
Copy link
Contributor

@pixie79 pixie79 commented Nov 29, 2023

Description

Adds pydantic as a requirement only if airflow version is 2.6.

Related Issue(s)

closes #654

Checklist

  • I have made corresponding changes to the documentation (if required)
  • I have added tests that prove my fix is effective or that my feature works

@pixie79 pixie79 requested a review from a team as a code owner November 29, 2023 08:52
@pixie79 pixie79 requested a review from a team November 29, 2023 08:52
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. area:dependencies Related to dependencies, like Python packages, library versions, etc labels Nov 29, 2023
@pixie79 pixie79 mentioned this pull request Nov 29, 2023
Closed
2 tasks
@pixie79
Copy link
Contributor Author

pixie79 commented Nov 29, 2023

First attempt failed when I ran the test. This second version passed the following sets of tests:

  • hatch run tests.py3.10-2.7:test-integration
  • hatch run tests.py3.10-2.6:test-integration

@jlaneve
Copy link
Collaborator

jlaneve commented Nov 29, 2023

@pixie79 I may be missing something here, but doesn't this add Pydantic as a dependency for all versions of Airflow? Or where is the logic to restrict this to just Airflow 2.6?

@pixie79
Copy link
Contributor Author

pixie79 commented Nov 29, 2023

It adds it as optional rather than required which allows both sets of test to pass. Also then allows the restrictions on mwaa to work.
As far as it would appear there is no way to list it as only optional for 2.6. I did look at adding it lower in the texts section where it would only get installed for 2.6 but that doesn’t seem correct to just have it for a test otherwise it would not be needed at all

@jlaneve
Copy link
Collaborator

jlaneve commented Nov 30, 2023

It adds it as optional rather than required which allows both sets of test to pass. Also then allows the restrictions on mwaa to work. As far as it would appear there is no way to list it as only optional for 2.6. I did look at adding it lower in the texts section where it would only get installed for 2.6 but that doesn’t seem correct to just have it for a test otherwise it would not be needed at all

Got it, that makes sense to me. So we're essentially asking the user to decide for themself if they need Pydantic? And if they decide they do, they just change their requirement to astronomer-cosmos[...,pydantic]?

@pixie79
Copy link
Contributor Author

pixie79 commented Nov 30, 2023

Yup, which i think is fair, what is odd is by just doing that change the tests passed and I am not really sure why they should?

@tatiana
Copy link
Collaborator

tatiana commented Nov 30, 2023 •
edited
Loading

@jlaneve I think @pixie79 's is a fair assumption - and we can update the tests that need to use the pydantic restriction. Please let me know if you'd like help to follow up on this - I know you have a lot going on!

@tatiana tatiana added this to the 1.3.0 milestone Nov 30, 2023
@pixie79
Copy link
Contributor Author

pixie79 commented Dec 1, 2023

Does this or need to be switched to be against main rather than the remove-pydantic branch?

@tatiana
Copy link
Collaborator

tatiana commented Dec 1, 2023

@pixie79 this change, by itself, will break some integration tests. I think our CI is set to only run integration tests on PRs to the main branch; for this reason, we're not seeing it.

If you'd like to see the integration tests running and fix them - please go ahead and update your PR to be based on main. Once the tests in your PR to the main pass, we can close PR #654 and merge your fix.

Otherwise, you can keep your PR as it is a co-author on the original PR #654. This will mean an additional PR to fix the integration tests, which will break once your PR is merged to #654.

The first approach of making a PR in the main is probably more pragmatic; let us know your preference - we're happy either way.

@pixie79 pixie79 mentioned this pull request Dec 2, 2023
Merged
2 tasks
jbandoro and others added 2 commits December 4, 2023 11:14
@jbandoro
Speed up integration tests (astronomer#732)
a2c58f5 
Speed up the integration tests by caching the dag bag result.

Previously, for each parametrized dag run test, it would reparse all of
the dags, which takes a non-trivial amount of time to parse all of the
cosmos example dags.

On my local machine, the total time went from 1616s to 540s, which will
save a good amount of GH minutes 😃.
@tatiana
Merge branch 'main' into remove-pydantic
ca76ea5
@tatiana
Fix functools cache test issue (astronomer#742)
e9950d7 
Fix exceptions raised for some versions of Python (e.g. 2.4) after
prematurely merging astronomer#732, leading to the main branch becoming red
```
../../../.local/share/hatch/env/virtual/astronomer-cosmos/Za_bFbg4/tests.py3.8-2.5/lib/python3.8/site-packages/_pytest/assertion/rewrite.py:186: in exec_module
    exec(co, module.__dict__)
tests/test_example_dags_no_connections.py:4: in <module>
    from functools import cache
E   ImportError: cannot import name 'cache' from 'functools'
```
@tatiana
Merge branch 'main' into remove-pydantic
6849341
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Dec 4, 2023
@tatiana tatiana closed this in #736 Dec 4, 2023
tatiana pushed a commit that referenced this pull request Dec 4, 2023
Optional pydantic (#736)
abeb15c 
Converts pydantic to being optional, as it is required for Airflow 2.6
but not others.

Closes #725 
Closes #654
arojasb3 pushed a commit to arojasb3/astronomer-cosmos that referenced this pull request Jul 14, 2024
@arojasb3
Optional pydantic (astronomer#736)
7456681 
Converts pydantic to being optional, as it is required for Airflow 2.6
but not others.

Closes astronomer#725 
Closes astronomer#654
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
area:dependencies Related to dependencies, like Python packages, library versions, etc size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@pixie79 @jlaneve @tatiana @jbandoro