Replace psutil with resource

[haz]

This is the first step towards having tarski work in the browser. A merge + version bump on pip would be tremendously awesome. Cheers!

[haz]

I realize Travis is complaining. Note https://github.com/cptpcrd/pypsutil/issues/1

Any chance 3.6 can be removed as a supported version? Seems to be fine for 3.7+

[gfrances]

Hi @haz, great to see you guys using Tarski! About this: besides the Python 3.7+ requirement, a concern is that pysutil doesn't seem to have support for windows platforms. I haven't personally been using the library on Windows, but I seem to remember that other people have (@miquelramirez ?). Could you describe a bit what's the issue the PR is solving for your intended use case, to see if we can work out some solution?

[miquelramirez]

I haven't personally been using the library on Windows, but I seem to remember that other people have (@miquelramirez ?)

I tried to run tarski on Windows like in '19... haven't tried again since.. I use tarski regularly on WSL but that doesn't count as "windows" :(

I don't think it is prudent to drop support for a platform right away. Is there a workaround for the platform issue?

[haz]

I guess I should have left more context...

Imagine being able to use the power of tarski in the browser for something like a fully client side plugin on the planning.domains editor. psutil is the only thing standing in the way of the core functionality.

@camcunningham already has it working with pypsutil, but swapping that out breaks 3.6 compatibility. gringo integration is being looked at next.

Judging by the limited scope of the PR (i.e., the limited use of psutil), could it (the required functionality) not just be replaced entirely?

I don't like the idea of maintaining an "online fork", as contributed support tends to drift.

[miquelramirez]

Imagine being able to use the power of tarski in the browser for something like a fully client side plugin on the planning.domains editor. psutil is the only thing standing in the way of the core functionality.

That'd be great stuff @haz, but I do not think it is reasonable to just say "install WSL" to people that want to use Anaconda on Windows.

To be honest, I wasn't even sure why tarski was depending on psutil. Looking at the use case I think the sensible way forward is to replace the use of psutil for that of the standard Python library resource. Then, we could have

import resource

class Timer:
    def __init__(self):
        self.start_time = time.time()
        self.start_clock = self._clock()
        self.start_mem = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss

    @staticmethod
    def _clock():
        times = os.times()
        return times[0] + times[1]

    def __str__(self):
        current = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
        current_in_mb = current / (1024*1024)
        rss_in_mb = (current - self.start_mem) / (1024*1024)
        return "[%.2fs CPU, %.2fs wall-clock, diff: %.2fMB, curr:  %.2fMB]" % (
            self._clock() - self.start_clock,
            time.time() - self.start_time, rss_in_mb, current_in_mb)

Is the resource module (being a standard Python module) acceptable for the sandboxed environment provided by the web browsers you guys are considering?

[haz]

That's precisely what I was getting at with...

Judging by the limited scope of the PR (i.e., the limited use of psutil), could it (the required functionality) not just be replaced entirely?

Not sure if pypsutil rules out windows...it's just Python 3.6 support that breaks afaik. Either way, can you do a quick check @camcunningham to see if resource plays nice with the browser embed? (I assume it would)

@miquelramirez Want the PR to replace it from our end, or would you guys like to handle it?

[miquelramirez]

No worries @haz the thing is that I wasn't sure which was the functionality in question until I had time to review the PR properly.

I think the cleanest thing is that you guys update the PR once you have tested the proposed replacement, and then it can all be merged from the web interface.

[haz]

@camcunningham On it?

[gfrances]

Yep, according to their docs, pypsutil does not support Windows. I agree - if the resource module is an acceptable solution for the browser architecture, let's go for that! I'd be curious to know what exactly you guys are using to run that in the browser - I assume some javascript Python interpreter?

[haz]

I agree - if the resource module is an acceptable solution for the browser architecture, let's go for that!

On its way...

I'd be curious to know what exactly you guys are using to run that in the browser - I assume some javascript Python interpreter?

Pyodide. It's magic ;).

[camcunningham]

Going to make the resource change and push soon

[haz]

@gfrances Can you trigger another Travis check?

[haz]

Perfecto!

[cptpcrd]

Maintainer of pypsutil here. I'd like to note a few things:

• Windows support for pypsutil is in the works, but it's still a ways off. 3.6 support is in master and will be released soon.
• pypsutil v0.1.0 doesn't have memory_info(); that was added later and will also be released soon. (It shows up in the docs because the "latest" docs build off of master.) I'm guessing your tests probably pass because they don't exercise the Timer class.
• The resource module is in the standard library; there is no need to add it to the requirements list.
• The resource module is NOT available on Windows. If you need to support Windows, AFAIK psutil is currently your only option to do this.

[gfrances]

Thanks guys! Actually, looking into the changes, the PR changes the actual functionality of our code, as before, the Timer class reported current memory consumption, whereas the Python resource module only seems to offer data on the maximum resident set size used (e.g. according to the man page). Other implementations I see online resort to fetching the information from a read of /proc/*, which I guess won't be available in the browser...

[haz]

Oomph...what about a fallback on the import? Only get the memory usage if you're on a system that handles psutil and wrap the import in a try/catch? I guess that only flies if you remove it from the requirements and expect it to be installed on the system using tarski?

[gfrances]

Thanks @cptpcrd for the info. You're right that the Timer class is not really properly tested for. It does indeed look like falling back to providing no memory info when on an unsupported platform could be the best alternative. As for the package dependencies, I've never tried this, but looking at the setuptools docs, looks like the way to go would be to use something like:

    install_requires=[
        "psutil;platform_system=='Windows'"
        ]

[gfrances]

I can try to have a go at this, if you guys don't mind

[haz]

Awesome, yes please!

[miquelramirez]

Thanks @cptpcrd for the very useful comments.

I just see the PR is directed to tarski master, could it be redirected to devel?

[haz]

I just see the PR is directed to tarski master, could it be redirected to devel?

That'd be a question for @gfrances

We care not who's github username is attached to the commit logs. We just want to minimize the headache of having this embedded in the browser ;).

[gfrances]

No worries about that, I'll go with a commit directly against the devel branch.

[gfrances]

@haz , @camcunningham, what do you guys use to test this? Do you have any sample travis or similar CI environment to show how to set up the testing environment for Pyodide? I pushed some changes into a psutil branch, but should test this works on your side it before anything else, as for instance I'm not sure what the value of the platform_system "environment marker" will be in that environment.

What is anyway the expected workflow for your application, if I may ask? Is the package precompiled somehow in webassembly on the web server? Is that done on the fly in the user's browser? (just trying to figure out whether you'll go through Pypi, or just run setup.py yourselves, etc.).

[haz]

If we can avoid rolling our own, all the better. So far it's been building our own wheel that does the trick, but straight from pypi is the dream. It's why we put in a PR in the first place.

[gfrances]

Sounds good. It'd still be good for me to know how to test these changes in the new branch work in your environment.

[camcunningham]

@gfrances currently the testing is purely functional: Getting tarski to load within browser via pyodide's micropip install, then running the some of the code on the docs Getting started portion to see that it's working properly at a glance.

Our focus thus far has been getting tarski to load within the pyodide environment within the browser, and not testing much of the libraries functionality yet.

[gfrances]

I see. Is there any script / steps you can share that I could use to test this is working then? Or could you have a go yourself with the psutil branch? (which os not in pypi yet)

[camcunningham]

I'll have a go at it. Just for reference, the steps that I've been taking are as follows: Need a wheel hosted locally to enable pyodide to load the package (since the changes aren't live via pypi)

1. Need to install some packages for wheel compilation: pip install wheel setuptools
2. On the changed tarski package, run python setup.py bdist_wheel, which will generate a whl file in a generated dist folder within the project.
3. Using a local server (I'm using an Apache server), host the file so that it can be accessed by pyodide via browser.
4. Run pyodide in an html file hosted on the server. This uses micropip to load in the pure python wheel.

Note: The html file also has to be hosted via a server so that we don't run into any CORS issues.

I've been using this code to determine if we have tarski up and running within pyodide.

[camcunningham]

@gfrances On first glance it seems to be working!

[gfrances]

I've released all changes so far into release 0.7.0, so that you guys can test whether pulling from pypi works. I'll close this PR for now, but feel free to reopen if the thing is still not fully functional, which I wouldn't rule out; in that case we can release some patched version.