Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2055439942 Thank you @potiuk :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk merged PR #38111: URL: https://github.com/apache/airflow/pull/38111 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2054164536 The docker example error workarounded in main. Merging -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2052628759 some main errrs fixed already - you will need to rebase -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2051971605 > Those are new tests you addded that are now failing, It's a different error you get: > > You also need to mark the test as db_test per instruction: > > > FAILED tests/providers/microsoft/azure/operators/test_msgraph.py::TestMSGraphAsyncOperator::test_execute - airflow.exceptions.AirflowInternalRuntimeError: Your test accessed the DB but `_AIRFLOW_SKIP_DB_TESTS` is set. > > Either make sure your test does not use database or mark the test with `@pytest.mark.db_test` > > See https://github.com/apache/airflow/blob/main/contributing-docs/testing/unit_tests.rst#best-practices-for-db-tests on how to deal with it and consult examples. > > Now why they are not failing in 3.10+ I am not sure - maybe your tests follow different path on those python versions without trying to access DB ? Hmm those tests are not new, the only new one is the TestMSGraphSensor and that one doesn't fail apparently. Weird... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2051965928 Those are new tests you addded that are now failing, It's a different error you get: You also need to mark the test as db_test per instruction: > FAILED tests/providers/microsoft/azure/operators/test_msgraph.py::TestMSGraphAsyncOperator::test_execute - airflow.exceptions.AirflowInternalRuntimeError: Your test accessed the DB but `_AIRFLOW_SKIP_DB_TESTS` is set. Either make sure your test does not use database or mark the test with `@pytest.mark.db_test` See https://github.com/apache/airflow/blob/main/contributing-docs/testing/unit_tests.rst#best-practices-for-db-tests on how to deal with it and consult examples. Now why they are not failing in 3.10+ I am not sure - maybe your tests follow different path on those python versions without trying to access DB ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2051846148 > See how it it's done in main (and you rebased to it) `pytestmark` is assigned at the whole module level`- so all those tests are now`db_tests` - your previous attempt removed all of the existing markers makiung them more susceptible to pytest-xdist/async bug we seem to be hitting Ok then I don't understand why sometimes it's still failing, it's not alway the case as on some python version it succeeds but on others not -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2051805887 See how it it's done in main (and you rebased to it) `pytestmark` is assigned at the whole module level` - so all those tests are now `db_tests` - your previous attempt removed all of the existing markers makiung them more susceptible to pytest-xdist/async bug we seem to be hitting -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2051782973 > You should remove your fix and rebase - in change you applied you removed the markers on tests but it needs `pytestmark` on the module level instead, So I need to do like this then, not completely understanding what you meant (I reverted my fix containing the db markers on the test-methods): ``` @pytest.mark.db_test class TestMSGraphAsyncOperator(Base): def test_execute(self): ... ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2051667128 You should remove your fix and rebase - in change you applied you removed the markers on tests but it needs `pytestmark` on the module level instead, -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2051052690 I will just add an example for the MSGrapSensor and how we use it with the PowerBI api -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2050548933 > I told you adding local imports is **really** not a good idea - those imports are needed elsewhere (in unit tests where they are mocked). Yeah saw it too, moved them at top of file now -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2050548094 > Surely, but we have the same problem with common code. If we add it today, it's only going to be really usable 10 months from now. because providers are importing stuff from earlier airflow versions that won't have it. We could already forsee the common method, and apply where possible and place # TODO: stating those should be refactored with common method once at certain Airflow version? Just thinking how we could handle it, even if it will only be possible to finalize it within 10 months, at least we can lay the foundation for a better and cleaner code in the future. Just thinking out loud here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2050393302 I told you adding local imports is **really** not a good idea - those imports are needed elsewhere (in unit tests where they are mocked). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2050266922 @potiuk FYI: I've also deleted the apache-airflow-providers-msgraph package under pypi -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2050247170 > > But if you confirm this is fine then no problem I will do it like this :) > > Yes. We do it in a number of other places - you already looked at it. > > > couldn't we adapt the verify_providers module as that is the one that causing the import issue and check that if the imported module isn't an ignored providers from BASE_PROVIDERS_COMPATIBILITY_CHECKS? > > Probably in this particular case it would work - but there are other imports that are ignored for similar reasons - not only for "transfer" operators. Example from aws security manager: > > ``` > try: > from airflow.www.security_manager import AirflowSecurityManagerV2 > except ImportError: > raise AirflowOptionalProviderFeatureException( > "Failed to import AirflowSecurityManagerV2. This feature is only available in Airflow versions >= 2.8.0" > ) > ``` > > So the "optional feaature" is used for more things and here we can use it easily. > > Adding logic to handle import for excluded provider might add even more complexity and needs someone to do it :) - but yes if you would like to do it now for example that woudl work, you'd just need to pass the excluded providers to the inside of the container image where the providers built are installed in. The providers are imported inside the image that has all 690+ dependencies installed, because otherwise a lot of them fail, so the import is done inside Breeze image, and it adds a bit of complexity. Adding three imports sounds easier (and likely can take a lot less time). But if you would like to do it - go for it, I just feel you already spent a lot of time on this one :) > > Some more comments: > > * using local imports is one thing that we want to avoid - because this is where compatibility checks will stop detecting errors altogether. and in your example you could even simply not wrap it in optionalfeature exception. In this case we **know** where the error comes from so we can add it. > > > with optional_provider_feature(): > > Surely, but we have the same problem with common code. If we add it today, it's only going to be really usable 10 months from now. because providers are importing stuff from earlier airflow versions that won't have it. I will do it the quick and fast way as you showed for now so that we can close this one. Yeah that's what I thought afterwards also, the dependency issue. Could be nice though, could even have a decorator for this which would make it even nicer, will see... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2050167395 > But if you confirm this is fine then no problem I will do it like this :) Yes. We do it in a number of other places - you already looked at it. > couldn't we adapt the verify_providers module as that is the one that causing the import issue and check that if the imported module isn't an ignored providers from BASE_PROVIDERS_COMPATIBILITY_CHECKS? Probably in this particular case it would work - but there are other imports that are ignored for similar reasons - not only for "transfer" operators. Example from aws security manager: ``` try: from airflow.www.security_manager import AirflowSecurityManagerV2 except ImportError: raise AirflowOptionalProviderFeatureException( "Failed to import AirflowSecurityManagerV2. This feature is only available in Airflow versions >= 2.8.0" ) ``` So the "optional feaature" is used for more things and here we can use it easily. Adding logic to handle import for excluded provider might add even more complexity and needs someone to do it :) - but yes if you would like to do it now for example that woudl work, you'd just need to pass the excluded providers to the inside of the container image where the providers built are installed in. The providers are imported inside the image that has all 690+ dependencies installed, because otherwise a lot of them fail, so the import is done inside Breeze image, and it adds a bit of complexity. Adding three imports sounds easier (and likely can take a lot less time). But if you would like to do it - go for it, I just feel you already spent a lot of time on this one :) Some more comments: * using local imports is one thing that we want to avoid - because this is where compatibility checks will stop detecting errors altogether. and in your example you could even simply not wrap it in optionalfeature exception. In this case we **know** where the error comes from so we can add it. > with optional_provider_feature(): Surely, but we have the same problem with common code. If we add it today, it's only going to be really usable 10 months from now. because providers are importing stuff from earlier airflow versions that won't have it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2050135785 > Some more explanation: This is what compatiblity checks are doing basically - they try to import the providers in airflow 2.6.0 so if you import anything that is missing, they should fail. Wrapping the failing import in "OptionalFeature" exception is a signal - "Yes, it's ok for this import to fail, it just means that something optoinal is missing" - hard to detect/decide automatically if missing import is something "wrong" or if this is just optional feature missing. Thanks for all the help :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2050133314 > > Now I'm getting this [errors](https://github.com/apache/airflow/actions/runs/8648788861/job/23713776144?pr=38111#step:10:2621) probably something else needs to be changed/excluded so it's not run in 2.6.0 anymore? > > Yeah. One more small thing - the check attempt to import provider code (and in this case they attempt to import transfer operators from GCS that are importing azure hooks (and the hooks are not there) - so > > BAsically you need to wrap the failing imports with AirflowOptionalProviderFeatureException like described here: > > https://github.com/apache/airflow/blob/main/airflow/providers/MANAGING_PROVIDERS_LIFECYCLE.rst#additional-changes-needed-for-cross-dependent-providers > > Yes. Not straightforward, but it's close Thanks Jarek, couldn't we adapt the verify_providers module as that is the one that causing the import issue and check the if the imported module isn't an ignored providers from BASE_PROVIDERS_COMPATIBILITY_CHECKS? Or will it fail further down anyway? I ask this a I don't feel very comfortable doing a try/except of an ImportError in like production code, because that would mean I would have to adapt that in the azure_blob_to_s" module for example, it would then become this if I'm not mistaken: ``` from __future__ import annotations import os import tempfile from typing import TYPE_CHECKING, Sequence from airflow.models import BaseOperator from airflow.providers.amazon.aws.hooks.s3 import S3Hook try: from airflow.providers.microsoft.azure.hooks.wasb import WasbHook except ModuleNotFoundError as e: from airflow.exceptions import AirflowOptionalProviderFeatureException raise AirflowOptionalProviderFeatureException(e) if TYPE_CHECKING: from airflow.utils.context import Context class AzureBlobStorageToS3Operator(BaseOperator): ``` But if you confirm this is fine then no problem I would do it like this :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2050116244 Some more explanation: This is what compatiblity checks are doing basically - they try to import the providers in airflow 2.6.0 so if you import anything that is missing, they should fail. Wrapping the failing import in "OptionalFeature" exception is a signal - "Yes, it's ok for this import to fail, it just means that something optoinal is missing" - hard to detect/decide automatically if missing import is something "wrong" or if this is just optional feature missing. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2050112072 > Now I'm getting this [errors](https://github.com/apache/airflow/actions/runs/8648788861/job/23713776144?pr=38111#step:10:2621) probably something else needs to be changed/excluded so it's not run in 2.6.0 anymore? Yeah. One more small thing - the check attempt to import provider code (and in this case they attempt to import transfer operators from GCS that are importing azure hooks (and the hooks are not there) - so BAsically you need to wrap the failing imports with AirflowOptionalProviderFeatureException like described here: https://github.com/apache/airflow/blob/main/airflow/providers/MANAGING_PROVIDERS_LIFECYCLE.rst#additional-changes-needed-for-cross-dependent-providers Yes. Not straightforward, but it's close -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2049944633 > "microsoft-azure" -> "microsoft.azure" : this is a short id we are using for providers - with `.` -> see "common.io". Likely PR where we normalize it would be good. Now I'm getting this [errors](https://github.com/apache/airflow/actions/runs/8648788861/job/23713776144?pr=38111#step:10:2621) probably something else needs to be changed/excluded so it's not run in 2.6.0 anymore? ``` Importing package: airflow.providers.atlassian.jira.operators Importing package: airflow.providers.atlassian.jira.sensors Exception when importing: airflow.providers.amazon.aws.transfers.azure_blob_to_s3 Traceback (most recent call last): File "/opt/airflow/scripts/in_container/verify_providers.py", line 199, in import_all_classes _module = importlib.import_module(modinfo.name) File "/usr/local/lib/python3.8/importlib/__init__.py", line 127, in import_module return _bootstrap._gcd_import(name, package, level) File "", line 1014, in _gcd_import File "", line 991, in _find_and_load File "", line 975, in _find_and_load_unlocked File "", line 671, in _load_unlocked File "", line 843, in exec_module File "", line 219, in _call_with_frames_removed File "/usr/local/lib/python3.8/site-packages/airflow/providers/amazon/aws/transfers/azure_blob_to_s3.py", line 26, in from airflow.providers.microsoft.azure.hooks.wasb import WasbHook ModuleNotFoundError: No module named 'airflow.providers.microsoft.azure' Exception when importing: airflow.providers.google.cloud.transfers.adls_to_gcs Traceback (most recent call last): File "/opt/airflow/scripts/in_container/verify_providers.py", line 199, in import_all_classes _module = importlib.import_module(modinfo.name) File "/usr/local/lib/python3.8/importlib/__init__.py", line 127, in import_module return _bootstrap._gcd_import(name, package, level) File "", line 1014, in _gcd_import File "", line 991, in _find_and_load File "", line 975, in _find_and_load_unlocked File "", line 671, in _load_unlocked File "", line 843, in exec_module File "", line 219, in _call_with_frames_removed File "/usr/local/lib/python3.8/site-packages/airflow/providers/google/cloud/transfers/adls_to_gcs.py", line 27, in from airflow.providers.microsoft.azure.hooks.data_lake import AzureDataLakeHook ModuleNotFoundError: No module named 'airflow.providers.microsoft.azure' Exception when importing: airflow.providers.google.cloud.transfers.azure_blob_to_gcs Traceback (most recent call last): File "/opt/airflow/scripts/in_container/verify_providers.py", line 199, in import_all_classes _module = importlib.import_module(modinfo.name) File "/usr/local/lib/python3.8/importlib/__init__.py", line 127, in import_module return _bootstrap._gcd_import(name, package, level) File "", line 1014, in _gcd_import File "", line 991, in _find_and_load File "", line 975, in _find_and_load_unlocked File "", line 671, in _load_unlocked File "", line 843, in exec_module File "", line 219, in _call_with_frames_removed File "/usr/local/lib/python3.8/site-packages/airflow/providers/google/cloud/transfers/azure_blob_to_gcs.py", line 25, in from airflow.providers.microsoft.azure.hooks.wasb import WasbHook ModuleNotFoundError: No module named 'airflow.providers.microsoft.azure' Exception when importing: airflow.providers.google.cloud.transfers.azure_fileshare_to_gcs Traceback (most recent call last): File "/opt/airflow/scripts/in_container/verify_providers.py", line 199, in import_all_classes _module = importlib.import_module(modinfo.name) File "/usr/local/lib/python3.8/importlib/__init__.py", line 127, in import_module return _bootstrap._gcd_import(name, package, level) File "", line 1014, in _gcd_import File "", line 991, in _find_and_load File "", line 975, in _find_and_load_unlocked File "", line 671, in _load_unlocked File "", line 843, in exec_module File "", line 219, in _call_with_frames_removed File "/usr/local/lib/python3.8/site-packages/airflow/providers/google/cloud/transfers/azure_fileshare_to_gcs.py", line 27, in from airflow.providers.microsoft.azure.hooks.fileshare import AzureFileShareHook ModuleNotFoundError: No module named 'airflow.providers.microsoft.azure' ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2049873688 > "microsoft-azure" -> "microsoft.azure" : this is a short id we are using for providers - with `.` -> see "common.io". Likely PR where we normalize it would be good. OMG why did I see this with the common.io, yeah maybe a normalize would be good, thanks anyway 👍 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2049820196 https://github.com/apache/airflow/pull/38936 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2049801878 "microsoft-azure" -> "microsoft.azure" : this is a short id we are using for providers - with `.` -> see "common.io". Likely PR where we normalize it would be good. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2049780531 > > it can become a dependency hell > > BTW. Yes. It's a bit contained hell and the rules we have about min-airflow version and separting providers out is our way of dealing with it. > > As you noticed, it makes contributions harder for sure - and review and keeping back-compatiblity is a real burden. > > However it makes life of many of our users a lot easier. Many of our users are using lower version of providers with newest airflow or higher version of providers with older airflow to bypass some of the dependency issues they have with their own external dependencies. This also allows us to introduce breaking changes in providers independently of Airflow. As usual: trade-offs. Now I got other weird errors when building for airflow 2.6.0: -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2049444186 > > it can become a dependency hell > > BTW. Yes. It's a bit contained hell and the rules we have about min-airflow version and separting providers out is our way of dealing with it. > > As you noticed, it makes contributions harder for sure - and review and keeping back-compatiblity is a real burden. > > However it makes life of many of our users a lot easier. Many of our users are using lower version of providers with newest airflow or higher version of providers with older airflow to bypass some of the dependency issues they have with their own external dependencies. This also allows us to introduce breaking changes in providers independently of Airflow. As usual: trade-offs. Indeed always have to make compromises, there aren't magic solutions that fit all unfortunately. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2049423633 > it can become a dependency hell BTW. Yes. It's a bit contained hell and the rules we have about min-airflow version and separting providers out is our way of dealing with it. As you noticed, it makes contributions harder for sure - and review and keeping back-compatiblity is a real burden. However it makes life of many of our users a lot easier. Many of our users are using lower version of providers with newest airflow or higher version of providers with older airflow to bypass some of the dependency issues they have with their own external dependencies. This also allows us to introduce breaking changes in providers independently of Airflow. As usual: trade-offs. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2049370288 > Alright, thank you Jarek, indeed it can become a dependency hell when you have to deal with so much dependencies. Will update this in the provider.yaml I was trying to reproduce it with breeze locally but couldn't get the image to build so was a bit stuck there so thanks again for the pointer. You also need to exclude it in the constants I pointed to and run `pre-commit` to update the dependencies in generated provider deps (as instructed in failed build) - we do not have an automated exclusion there (some day maybe but it has a bit of complexity as there might be variations), you need to manually add it there, otherwise you will get a conflict when installing it (but this time it should be fast). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2049360149 > Hey @dabla > > I think you should update `apache-airflow>=2.7.0` in mysql-azure provider and add `mysql-azure` to the https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/global_constants.py#L470 for 2.6.0. The problem you see in the compatibility check that the library you added is conflicting with **some** libraries used in 2.6.0 and `pip` times out on trying to find a resolution of dependencies when installing new `mysql` provider for 2.6.0. This is one of the problems of having so many dependencies with so many providers that it does not really work for all combinations. > > We are about to bump `min-airflow` version to 2.7.0 for all providers as of 27th of April : https://github.com/apache/airflow/blob/main/PROVIDERS.rst#upgrading-minimum-supported-version-of-airflow and it's very likely next wave of providers will have 2.7.0 as minimum (but we also allow to bump min airflow version individually for providers if there is a need). Alright, thank you Jarek, indeed it can become a dependency hell when you have to deal with so much dependencies. Will update this in the provider.yaml I was trying to reproduce it with breeze locally but couldn't get the image to build so was a bit stuck there so thanks again for the pointer. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2049355078 Hey @dabla I think you should add `apache-airflow>=2.7.0` in mysql provider and add mysql to the https://github.com/apache/airflow/blob/main/dev/breeze/src/airflow_breeze/global_constants.py#L470 for 2.6.0. The problem you see in the compatibility check that the library you added is conflicting with **some** libraries used in 2.6.0 and `pip` times out on trying to find a resolution of dependencies when installing new `mysql` provider for 2.6.0. This is one of the problems of having so many dependencies with so many providers that it does not really work for all combinations. We are about to bump `min-airflow` version to 2.7.0 for all providers as of 27th of April : https://github.com/apache/airflow/blob/main/PROVIDERS.rst#upgrading-minimum-supported-version-of-airflow and it's very likely next wave of providers will have 2.7.0 as minimum (but we also allow to bump min airflow version individually for providers if there is a need). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2040760143 I ma afraid conflicts need to be resolved with rebase. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2024146526 > > You should **just** rebase. "Always Be Rebased" as the old Chinese proverb says. > > Who would be kind enough to review this PR? I just can't figure out what's wrong the the docs build... Som indentation issues in your docstrings. Sphinx is very picky about these. You can builds the docs locally with `breeze build-docs` - it will also generate the temporary .rst pages generated from the docstring that you can inspect to check things out. There are few other things (unit tests) : * event_loop seems to be closed where it should not (or at least so the description says) * a connection is defined twice apparently * some warnings are generated when the Hooks are imported * the compatibility checks (or at least some part of them are being fixed in #38545 so you do not have to worry about them until it's merged and you rebase -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2022384426 > You should **just** rebase. "Always Be Rebased" as the old Chinese proverb says. Who would be kind enough to review this PR? I just can't figure out what's wrong the the docs build... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
potiuk commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-2009209802 You should **just** rebase. "Always Be Rebased" as the old Chinese proverb says. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Adding MSGraphOperator in Microsoft Azure provider [airflow]
dabla commented on PR #38111: URL: https://github.com/apache/airflow/pull/38111#issuecomment-1999330233 Why are there suddenly tests failing of classes I didn't even touch? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
