potiuk commented on PR #46498: URL: https://github.com/apache/airflow/pull/46498#issuecomment-2639057193
Yeah. Some problems are expected - especially for such huge providers as amazon. This is one of the reasons we did no move all providers at once, because rather than havin 10 people solving those things individually, we would have to have 1 person solving all 100 providers issues at once, we are basically crowd-sourcing problem solving. But I see my role here as help to guide people (and I did that in other providers) how to solve those difficult issues. That's a good learning for everyone as well. Worked so far very well. and I am happy to guide you as well. > 1) mypy failures: Yes. It's known that `mypy` is not fully deterministic and might or migh not find different errors when files are moved. Happened many times for others who moved other providers. Seems that pretty much all the problems you see are the same -> the optional part of event is not automatically detected by MyPy now. Why exactly it has not detected it before - I think it's a good question for MyPy creators (basically at this stage we are waiting until `astral` will come up with their own type hint consistency checker, because MyPy is plagued by all kinds of those problems and we hit it many times But it seems in this case (or at least when I googled it) setting `allow_redefinition` should solve the problem https://mypy.readthedocs.io/en/stable/config_file.html#confval-allow_redefinition -> this is one of the features of `mypy` that you can enable ot allow redefining type of variable. It might - of course - cause a ripple effect and MyPy complaining about something else, but it's worth trying if you do not want to `fix` the way things are implemented now. Because I personally think that redefining type of the variable like that is a bit fishy (that's why also it's not enabled by default by MyPY) - I would rather create another variable (`coerced_event` would be a good name) and use this one instead of `event` - but If you do not want to refactor it in all those places (should be relatively easy with IntelliJ or VSCode refactoring - just rename the variable with refactor and bring `event` back in the method signature and `validate_execute_complete_event` method call withou t refactoring. Yep. A bit "dull" change, and will take about 10-15 minutes of time to change it in all those places, but possibly it's worth it. > 2) docs > There are many _api docs which either seem out of date or have spelling errors that seem to already be fixed. I do not know how to generate these docs. I tried a docs build but that fails because all paths to system test examples are now incorrect. I'm not sure if this is something unique to amazon, or if the migration script failed (there were some issues related to the aws directory within the amazon provider). Unfortunately when `autoapi` generates API docs and fails, no API docs are generated and then you can have many unrelated errors about missing docs. Not much we can do about it. This is how sphinx/autoapi works. The errors are likely coming from just a few failing docstring generation by autoapi - likely those last errors are the root cause - those classes are referred to in some docstrings you just need to fin them: `tests.system.providers.amazon.aws.example_appflow_run.create_s3_to_s3_flow`. The migration script does not handle **all** cases. It handled "most common cases" - 95% of those - simply those that could be easily automated, common for all operators and are not risky - does not require human to diagnose and fix those. The rest of fixes for providers have to be done manually. That's one of the tasks when migrating provider to identify and fix those remaing 5%. If we could 100% migrate all providers, I would not ask others for help, I would just run the scripts myself. Unfortunately, with inter-relations between providers, doc generation and others, automating absolutely all cases would take likely far more effort of one person to track, identify and fix those. The premise was that individuals looking at those will do it much faster (and in parallel or at different times) - also that their involvement in helping to move will make it more of a "community" effort. Which pretty much worked so far. > 2) there are many bits of code that have not been static checked for issues of spelling and words Yep. We are well aware that things are not always fixable for those. There are exclusions in .pre-commit-config.yaml for thsoe. Likely those need to be moved. Like the exclusions should be updated - for example here: https://github.com/apache/airflow/blob/main/.pre-commit-config.yaml#L661 rather than the words removed > Other problems: * Breeze unit tests -> There are also some unit tests to fix in Breeze -> but those are mainly about changed path * Check if RST files use double backticks for code............................Failed -> looks like some of the ``api`` generated docs contain single backticks - which for some reason was excluded so far, but now we seem to see it. Those are easy to fix and I think it's good we started to detect those. * nohup.out is accidentally merged. * some system tests are failing - likely there are some imports that have not been covered by the migration script - but should be as easy as search/replace for those. I hope we can get it green quickly :) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
