You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/08/22 07:39:24 UTC
[GitHub] [airflow] potiuk opened a new pull request #10470: Move perf_kit to tests.utils
potiuk opened a new pull request #10470:
URL: https://github.com/apache/airflow/pull/10470
Perf_kit was a separate folder and it was a problem when we tried to
build it from Docker-embedded sources, because there was a hidden,
implicit dependency between tests (conftest) and perf.
Perf_kit is now moved to tests to be avaiilable in the CI image
also when we run tests without the sources mounted.
This is changing back in #10441 and we need to move perf_kit
for it to work.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-691115150
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #10470:
URL: https://github.com/apache/airflow/pull/10470
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-691115150
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-678630816
Yeah. I think I found 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-678710149
We could also let these imports fail. Perf_kit is only needed for the developer and not needed for the CI.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#discussion_r475199304
##########
File path: tests/utils/perf/perf_kit/__init__.py
##########
@@ -32,33 +32,34 @@
=======
Review comment:
Ah yeah. Fixing it 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-678626072
@mik-laj - I totally don't understand why those tests fail with 2 rather than expected number of runs:
```
AssertionError: The expected number of db queries is 21. The current number is 2.
```
Those tests are randomly failing also for older runs but here it is consistent once I moved the code to test/utils. Can you please take a look and see what could be wrong here/? It's rather difficult to figure out, but at least this is consistent now and we might have a chance to fix it permanently and remove the flakiness.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-691076610
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-691301221
> > The point is that `tests.utils` corresponds to `airflow.utils` and probably should have no other stuff. I will open PR
>
> I see, not too obvious though :( . I was never suspecting this. But I am not sure how to make it more obvious,
I think we should pay more attention to this in future. At least I will do 😄
<img width="1124" alt="Screenshot 2020-09-11 at 22 37 40" src="https://user-images.githubusercontent.com/9528307/92970711-6920c280-f47f-11ea-8c81-f4484b16e08e.png">
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-678633379
yeah, I found it :). I also had to move the perf dag path
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-691115150
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-691289451
> The point is that `tests.utils` corresponds to `airflow.utils` and probably should have no other stuff. I will open PR
I see, not too obvious though :( . I was never suspecting this. But I am not sure how to make it more obvious,
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-691513657
:rofl:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-678762991
I do not like the idea of failing imports, I have no idea whether there are no side effects and I think those tests woudl fail if the import fails, because there would be no way to count the queries. I even prefer to move it where it belongs.
Perf_kit it is essentially one of the test utils we have (currently).
If You intend to make it a standalone tool, you should make it a separate library on pypi and import it from there. I tink that might be a good idea to be honest - why don't you do it @mik-laj ?
I think perf-kit is a good candidate to release via PyPi as standalone tool. I think it would be rather useful for others. I'd really encourage you to do so.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#discussion_r475149950
##########
File path: tests/utils/perf/perf_kit/__init__.py
##########
@@ -32,33 +32,34 @@
=======
Review comment:
The previous section should be updated as it contains invalid paths.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-691115150
@turbaszek - because both dirs were there and my eye caught "utils" first I guess :)
Both directories were there when I moved it, so I guess arbitrarily I chose the "utils" one.
But feel free to merge those two. There is no difference from the build POV.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-691214596
The point is that `tests.utils` corresponds to `airflow.utils` and probably should have no other stuff. I will open PR
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-691076610
@potiuk why we put it in `utils` not `test_utils`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-678626957
OK. I think I know @mik-laj let me try smth next
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #10470: Move perf_kit to tests.utils
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #10470:
URL: https://github.com/apache/airflow/pull/10470#issuecomment-691076610
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org