Khan / gae_mini_profiler

A ubiquitous mini-profiler for Google App Engine, inspired by mvc-mini-profiler
http://bjk5.com/post/6944602865/google-app-engine-mini-profiler
271 stars 45 forks source link

Can we make this module installable via pip? #74

Open lipis opened 9 years ago

houmie commented 9 years ago

Totally agree, at this time and age, every project should support pip.
I just tried it via pip install https://github.com/Khan/gae_mini_profiler/archive/master.zip and it fails. same with pip install git+https://github.com/Khan/gae_mini_profiler.git

Please!

kolyaflash commented 9 years ago

I think we should replace "make installable" with "make downloadable" in case with GAE and taking into account how gae_mini_profiler integrates with app. And it sounds pretty senseless.

houmie commented 9 years ago

I think it would be very useful. Have a look at template projects such as https://github.com/kamalgill/flask-appengine-template or https://github.com/gae-init/gae-init, you will see that they use virtualenv for all packages. Personally I utilise make setup to install via pip and rsync the files to the lib folder. That way I keep the Lib clean of unnecessary egg files.

Hence having the option to install it via pip would be incredibly useful.

bogdanvarlamov commented 9 years ago

All it needs is a setup.py file, to be supported by pip, right?

jlfwong commented 9 years ago

If it were installable via pip, how would the static assets be used? i.e. how would you specify the path app.yaml to gae_mini_profiler/static?

lipis commented 9 years ago

By installing it to a specific folder?

pip install --install-option="--prefix=/path/to/gae/app" gae_mini_profiler
jlfwong commented 9 years ago

Is that option specifiable via a requirements.txt?

lipis commented 9 years ago

hmm.. actually I'm using it here: https://github.com/gae-init/gae-init-debug/tree/master/main/libx and the whole magic with pip was done by @gmist so maybe he can help more in this area :)

jlfwong commented 9 years ago

My gut feeling is that this isn't particularly useful unless it's specifiable via requirements.txt. If you have to do a manual pip install step and specify a target directory, then I'm not seeing much by way of significant benefit over using git clone or using a submodule.

For reference, at Khan Academy, we use it as a submodule of our main repository.

bogdanvarlamov commented 9 years ago

I'm installing everything from requirements.txt into a lib folder that I don't want in version control. Currently I have to manually add the gae mini profiler from git into this lib folder, or else have inconsistent import paths.

If it were a pip installable module, I wouldn't have to do this hacky workflow.

On Wed, Nov 11, 2015, 12:57 PM Jamie Wong notifications@github.com wrote:

My gut feeling is that this isn't particularly useful unless it's specifiable via requirements.txt. If you have to do a manual pip install step and specify a target directory, then I'm not seeing much by way of significant benefit over using git clone or using a submodule.

For reference, at Khan Academy, we use it as a submodule of our main repository.

— Reply to this email directly or view it on GitHub https://github.com/Khan/gae_mini_profiler/issues/74#issuecomment-155861515 .

lipis commented 9 years ago

Same here.. as everything else for GAE, the 3rd party libs needs to be included in the project's directory.. git modules are nice, but not very practical in real life :)

houmie commented 9 years ago

Yes, this is exactly my workflow as well. I have to copy it in manually into lib folder, and I don't want the .git folder in there.

jlfwong commented 9 years ago

Yes, this is exactly my workflow as well. I have to copy it in manually into lib folder, and I don't want the .git folder in there.

Sorry if I'm misunderstanding, but this isn't what I'm suggesting -- this is the role of git submodule.

In our main repository, for instance, we do this:

git submodule add git@github.com:Khan/gae_mini_profiler.git third_party/gae_mini_profiler

I'm installing everything from requirements.txt into a lib folder that I don't want in version control.

If I'm understanding correctly, this means you have pip install --install-option="--prefix=/path/to/gae/app", which installs all packages listed in requirements.txt as part of your dev environment setup + part of your deploy scripts? If that's the case, then I can see making this repo pip installable as pretty reasonable.

If that's the path that you're recommending, then the next step would be if someone would be so kind as to make a setup.py for this repo along with relevant documentation in the README indicating that you must use this in conjunction with --install-option, and submit that as a pull request.

bogdanvarlamov commented 8 years ago

Jamie,

You are exactly correct in how I'm using it (pip command as part of setup/deploy).

I've created a fork where I'm experimenting with creating the setup.py and manifest, etc.

I'll send a pull request if I can make it work. So far I can create an archive with ask the necessary files, so I'm close.

On Wed, Nov 11, 2015, 4:59 PM Jamie Wong notifications@github.com wrote:

Yes, this is exactly my workflow as well. I have to copy it in manually into lib folder, and I don't want the .git folder in there.

Sorry if I'm misunderstanding, but this isn't what I'm suggesting -- this is the role of git submodule.

In our main repository, for instance, we do this:

git submodule add git@github.com:Khan/gae_mini_profiler.git third_party/gae_mini_profiler

I'm installing everything from requirements.txt into a lib folder that I don't want in version control.

If I'm understanding correctly, this means you have pip install --install-option="--prefix=/path/to/gae/app", which installs all packages listed in requirements.txt as part of your dev environment setup + part of your deploy scripts? If that's the case, then I can see making this repo pip installable as pretty reasonable.

If that's the path that you're recommending, then the next step would be if someone would be so kind as to make a setup.py for this repo along with relevant documentation in the README indicating that you must use this in conjunction with --install-option, and submit that as a pull request.

— Reply to this email directly or view it on GitHub https://github.com/Khan/gae_mini_profiler/issues/74#issuecomment-155922655 .

lipis commented 8 years ago

Good stuff..! Looking forward @bogdanvarlamov.

@jlfwong what about the other 3rd party libraries?! Are you not using pip for them Khan Academy?

(In general it makes sense for you guys to have a git submodule as this is your library and you know what is going on, but depending on a git submodule in other projects makes it inconvenient to work, especially when it comes to versions. If you have everything in the requirements.txt it's much easier..)

jlfwong commented 8 years ago

@jlfwong what about the other 3rd party libraries?! Are you not using pip for them Khan Academy?

We are, but we have a bunch of restrictions imposed as a result of GAE's system for libraries: https://cloud.google.com/appengine/docs/python/tools/libraries27?hl=en and we run gae-mini-profiler (as well as many other third party libs) in production, so we can't use pip for those.

pip, for us, is only used for dev-only tools, and for things that happen to be a subset of the libraries listed on that page.

bogdanvarlamov commented 8 years ago

Hi All,

I created a branch in my fork of this repo that contains the refactoring needed to have this project be "pipable". It is available here: https://github.com/bogdanvarlamov/gae_mini_profiler/tree/pip_support

Since the changes are more extensive than I initially thought, I need @jlfwong to take a look and decide what is preferable:

1) pull the changes and re-organize the layout of this project to match what pip packages should be structured as -- if so, I need to know who/what to put in the license file for (c) info as well as the version number for the package (and author info for pip package as well) 2) create a separate pip-ready fork to be maintained apart from this main repo (maybe this repo can even be a submodule in that repo)

If anyone is curious about trying out my fork in their projects to make sure it works right, here are the steps:

1) Add this line to your requirements_dev.txt file: git+https://github.com/bogdanvarlamov/gae_mini_profiler.git@pip_support#egg=gae_mini_profiler 2) Install all dependencies as you normally would via pip install -r requirements_dev.txt -t src/lib

You can now import gae_mini_profiler packages into your code just like you would with any other package in the lib folder.

benjaminjkraft commented 8 years ago

@bogdanvarlamov Thanks for looking into this!

It looks like as-written this will require some changes to projects already using the mini profiler, because of the directory change. Do you know if there's a reasonable way to make it pip-installable without doing that, even if it's not exactly the standard/recommended approach? Doing the update is fairly easy for us, but I'd be concerned about breaking things for other sites that use it, especially since we don't really have any versioning at the moment. If we can do it without breaking things, we'd certainly be happy to accept a pull request and we can talk about the details of that then.

bogdanvarlamov commented 8 years ago

Unfortunately I spent a good bit of time trying different hacks to get a pip package without directory structures, but I couldn't get it to work.

Keep in mind that I've never made a pip install package before, and don't use Python daily, so I might just be an idiot and missing something an experienced person might know ;)

Out of the hacks I tried, the only plausibly working one was to explicitly list all files and where they must be copied, but it was so awful I abandoned that approach due to maintainability concerns.

On Tue, Dec 8, 2015, 8:29 PM Ben Kraft notifications@github.com wrote:

@bogdanvarlamov https://github.com/bogdanvarlamov Thanks for looking into this!

It looks like as-written this will require some changes to projects already using the mini profiler, because of the directory change. Do you know if there's a reasonable way to make it pip-installable without doing that, even if it's not exactly the standard/recommended approach? Doing the update is fairly easy for us, but I'd be concerned about breaking things for other sites that use it, especially since we don't really have any versioning at the moment. If we can do it without breaking things, we'd certainly be happy to accept a pull request and we can talk about the details of that then.

— Reply to this email directly or view it on GitHub https://github.com/Khan/gae_mini_profiler/issues/74#issuecomment-163077644 .