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

Support for blackd #23

Closed
DylanYoung opened this issue Jul 9, 2020 · 10 comments
Closed

Support for blackd #23

DylanYoung opened this issue Jul 9, 2020 · 10 comments
Labels
enhancement New feature or request

Comments

@DylanYoung
Copy link

Probably this requires a port argument and not much else after #22 is finished.

Then the arguments just need to be translated into an HTTP request with the appropriate X- headers.

Thoughts?

@akaihola
Copy link
Owner

akaihola commented Jul 9, 2020

Hi @DylanYoung,

Thanks for the idea!

According to documentation, the main benefit from using blackd instead of black to get rid of the startup overhead and have a faster response in IDEs.

darker currently imports black and calls it inside the Python process instead of invoking black as a shell command. Thus we only spend the Python startup overhead once.

If darker supported blackd, you'd still pay the startup overhead of darker and only get a small speedup from not having to import black.

To achieve similar fast response as with blackd, we would have to implement a darkerd and do API calls to blackd Python API calls to black from it.

Or am I missing a separate non-performance-related benefit from using blackd?

@akaihola
Copy link
Owner

On the topic of a potential darkerd implementation:

Let's consider a darkerd daemon which makes HTTP API calls to a separate blackd daemon. That would mean

  • two processes running,
  • added code complexity in darkerd, and
  • probably no performance benefits (compared to calling the black Python API directly in the darkerd process).

So the performance benefit would be achieved by just

  • creating a darkerd,
  • making it call the darker Python API, and
  • letting darker in turn call the black and (and optionally isort) Python APIs internally.

Compared to blackd, it's important to consider that it's not possible to pass only source code in the HTTP request. darker needs to have access to the unmodified version of each file in Git HEAD. Do do this,

  • the HTTP request to darkerd must point to actual files on disk (either Git root dir + set of relative paths, or just a set of absolute paths)
  • the darkerd process must have access to the filesystem, so in practice run in the same machine as the caller (i.e. the IDE)

If we were to implement darkerd, I would

  • mirror the implementation of blackd (it's under 200 lines of code)
  • use the same libraries (aiohttp, aiohttp-cors and maybe click)

How does this reasoning sound?

@Mystic-Mirage
Copy link
Collaborator

A lot of code without a valued benefit.
Does somebody really face performance issues?

@DylanYoung
Copy link
Author

DylanYoung commented Jul 10, 2020

@Mystic-Mirage Do you use darker in your editor? It takes like 30 seconds to format.

@DylanYoung
Copy link
Author

DylanYoung commented Jul 10, 2020

@akaihola That sounds reasonable to me. Sounds like the implementation of darker complicates things a bit compared to what I was thinking and I was neglecting isort, which necessitates not using blackd (if we also want to save the startup cost of isort).

You're correct of course about the startup; I wasn't thinking about the problem right.

@Mystic-Mirage
Copy link
Collaborator

@Mystic-Mirage Do you use darker in your editor? It takes like 30 seconds to format.

I'm a lucky guy and my darker takes 0.5~1.5 seconds. My IDE calls it on every save.

We need to profile darker and find bottlenecks.

@akaihola
Copy link
Owner

akaihola commented Jul 10, 2020

@Mystic-Mirage wrote:

A lot of code without a valued benefit.
Does somebody really face performance issues?

Well, I can appreciate the improved snappiness if we can get e.g. down from 1500 ms to 300 ms response time. And implementing a darkerd similar to the <200 LoC blackd/__init__.py isn't really that much – and it can be a completely separate package living in the same repository just like in black (or even a completely separate project if that's preferred).

@DylanYoung wrote:

I was neglecting isort, which necessitates not using blackd (if we also want to save the startup cost of isort)

Even without using isort, running a separate blackd instead of calling the Python API directly in darkerd will give zero performance benefit, and only adds more code and another running process to manage. I'm happy to be proven wrong, but I can't imagine any situation with a hypothetical darkerd where blackd brings any benefit to us compared to just black.

Note that in order to use darkerd, the IDE must be able to

  • make a request which includes the absolute path of the file to reformat, and
  • after getting the HTTP response it must be able to
    • either re-read the file from disk immediately
    • or replace the content with the reformatted version from the response.

I haven't checked which IDEs can be configured to do that – do you have any insight on this?

Do you use darker in your editor? It takes like 30 seconds to form

My experience is around 1–2 seconds, similar to @Mystic-Mirage's. How long does darker path/to/your/file.py take on the command line? Are you sure your editor reloads the file immediately after running darker? Could you run darker -v path/to/your/file.py and paste the output here (feel free to redact any sensitive content)?

Also, please make a copy of the problematic .py file as well as its unmodified version from your last Git commit so you can test changes in performance in case we profile and try to improve darker performance. Thanks!

@DylanYoung
Copy link
Author

I've had issues with my editor, so it's probably not all darker. I can't even get it to run at all right now, lol. I think profiling to start is the way to go. A darkerd would be nice though :)

Even without using isort, running a separate blackd instead of calling the Python API directly in darkerd will give zero performance benefit, and only adds more code and another running process to manage. I'm happy to be proven wrong, but I can't imagine any situation with a hypothetical darkerd where blackd brings any benefit to us compared to just black.

You're absolutely right. That's what I meant by second comment. Sorry that wasn't clear.

@akaihola
Copy link
Owner

@DylanYoung I'd like to change the title of this issue from "Support for blackd" to "Expose darker's functionality as an HTTP API with a darkerd server". Agree?

@akaihola akaihola changed the title Support for blackd Expose darker's functionality as an HTTP API with a darkerd server Aug 19, 2020
@akaihola akaihola changed the title Expose darker's functionality as an HTTP API with a darkerd server Support for blackd Aug 19, 2020
@akaihola
Copy link
Owner

I'm closing this issue in favor of #59.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants