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

Python Developer Experience #684

Merged
merged 17 commits into from
Oct 23, 2020
Merged

Python Developer Experience #684

merged 17 commits into from
Oct 23, 2020

Conversation

jrhizor
Copy link
Contributor

@jrhizor jrhizor commented Oct 22, 2020

This PR sets up for Gradle:

  • Python venv initialization in every Python project
  • Sets up import sorting (isort), formatting (black), static checking (mypy), and linting (flake8) -- all the configs here are stolen directly from @ChristopheDuong and work great together.
  • After running ./gradlew build, Python can be run locally, in IntelliJ, and has proper syntax highlighting in IntelliJ.
  • In yesterday's standup, I was talking about speed. That actually isn't a huge problem in this iteration. The previous version I was working on used pygradle which was cumbersome to use and slow to initialize.

In a separate PR we can easily add pytest testing (we could also set up tox similarly).

Caveats:
It's very hard to get Python working nicely in IntelliJ alongside Java.

I can get proper syntax highlighting to work while using symlinked dependencies, but it doesn't keep the module set as Python after you manually set it and then refresh Gradle unless you exclude the sources completely from the Java plugin in Gradle. Even then, you have to set a venv to work manually. What seems to work best is to have a single Python interpreter set up for the integration's venv you're working with. Then you can set all of the facets to that interpreter. If you need to switch interpreters, then you change the global interpreter. This definitely is ideal but I couldn't find any way to configure IntelliJ to autodetect and use subproject venvs for Python. If we recommend contributors create a Python project in PyCharm or IntelliJ to work with a single integration, this won't be a problem. It looks like this will be fixed soon in IntelliJ (hopefully):
- JetBrains/intellij-community#1419
- https://intellij-support.jetbrains.com/hc/en-us/community/posts/360004233639-Detect-venv-in-the-project-folder

Our best shot
For developing Python in IntelliJ, I think the best way is to create a new project from existing sources that doesn't use Gradle at all. This basically gives up on syntax highlighting in the Gradle view of IntelliJ. We'd have two projects locally, one for Python development and one for Java/Gradle development. You can still run the Python gradle commands from the IDE if yo want. Then, we can remove the symlinked dependencies in this PR.

The default way the Python projects are created without Gradle are as separate subprojects, each using their own correct venv and correct syntax highlighting. This means there's no manual set up at all in the new project window.

After more than a day of investigation, I think this is our best bet. I'm leaving the symlinks now if you want to play around with it in the Gradle IntelliJ project, but I'll remove it if we decide to go the direction of separate project windows in IntelliJ. Since there is no way to get automatic setup in the Gradle/IntelliJ view, I think this is the best option.

Reading order:

  • tools/gradle/commons/integrations/python.gradle
  • tools/python/*
  • other build.gradle files
  • everything else

'base_singer',
'airbyte_protocol'
]
"psycopg2==2.8.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

upgraded this because installing 2.7.4 fails locally on my local Mac/Py3.8

@jrhizor jrhizor requested a review from cgardens October 22, 2020 17:53
Copy link
Contributor

@cgardens cgardens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!
can you add into source somewhere a readme of how to get up and running with python? just synthesizing some of the stuff you mentioned. doesn't have to be long. but if i'm a new person to the team, it should be enough that i can get my second intellij instance running without asking for help.

[settings]
# This is to make isort compatible with Black. See
# https://black.readthedocs.io/en/stable/the_black_code_style.html#how-black-wraps-lines.
line_length=88
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this kinda small?

Copy link
Contributor

@sherifnada sherifnada Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

140 pls?

@@ -0,0 +1,46 @@
apply plugin: 'ru.vyarus.use-python'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice.

[settings]
# This is to make isort compatible with Black. See
# https://black.readthedocs.io/en/stable/the_black_code_style.html#how-black-wraps-lines.
line_length=88
Copy link
Contributor

@sherifnada sherifnada Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

140 pls?

from .source import *
from .source import TemplatePythonSource

__all__ = ["TemplatePythonSource"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own curiosity, why do we need that line?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To better support introspection, modules should explicitly declare the names in their public API using the all attribute.

Moreover, it solves flake8 warnings:
https://stackoverflow.com/a/49266468

@ChristopheDuong
Copy link
Contributor

Then, we can remove the symlinked dependencies in this PR.
The default way the Python projects are created without Gradle are as separate subprojects, each using their own correct venv and correct syntax highlighting. This means there's no manual set up at all in the new project window.
After more than a day of investigation, I think this is our best bet. I'm leaving the symlinks now if you want to play around with it i n the Gradle IntelliJ project, but I'll remove it if we decide to go the direction of separate project windows in IntelliJ. Since there is no way to get automatic setup in the Gradle/IntelliJ view, I think this is the best option.

Great setup with the symlinks!

I found adding an extra symlink in airbyte-integration to airbyte_protocol (maybe have to add one for singer-source too?) also allows you to open your IDE in the integration folder and navigate all connectors/templates at once quite nicely!

@jrhizor jrhizor merged commit b34d559 into master Oct 23, 2020
@jrhizor jrhizor deleted the jrhizor/python-dx branch October 23, 2020 19:43
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.

5 participants