Several misc. fixes, cleanups, features by ericaltendorf · Pull Request #21 · ericaltendorf/plotman

ericaltendorf · 2021-02-06T23:41:45Z

Fix logic for identifying plotting processes
Update sample config file to be valid and better commented
Add a job-progress histogram to the interactive display
Various minor fixes and cleanups

…t test

…rovements to interactive mode display

… plot jobs

altendky

Sorry, but I didn't get to a whole lot of review of correctness. Something I need to work on and also something that follows from 1) me being new to this code and 2) us being new to each other's 'style' preferences so that stuff jumps out etc. Hopefully the explanations are either things you don't know or aspects you hadn't considered.

Compound PRs addressing multiple things generally end up being way harder to review since the reviewer has to dissect which bit goes with what. Also, it means you are pretty likely to have several things held up from getting merged over some discussion related to just one little bit of the PR.

class Log:
    entries = []
    cur_pos = 0

I personally use https://attrs.org for pretty much all of my classes. But, to this approach... Those aren't defining attributes of the class instances, but attributes of the class itself. Effectively globals that are just accessed via the class. If you self.cur_pos = 3 then you are adding an instance attribute shadowing the class attribute. If you self.entries.append(3) you are mutating the single class attribute, not an instance specific list. So, all instances will see that new value on the end of the list. This is related to the common mutable function parameter default confusion.

FYI about GitHub reviews and suggestions. If you like a suggestion exactly as is you can either commit it individually or group it together with others and commit them in a batch by clicking the corresponding buttons. It can be convenient for you and, in cases where you care, it lists the suggester as an author on the commit as well. (I don't care for my contributions but would for other people's)

You've got tests now. Add CI next. :]

altendky · 2021-02-07T00:18:55Z

+        # TODO: handle resizing.  Need to (1) figure out how to reliably get
+        # the terminal size -- the recommended method doesn't seem to work
+        #    (n_rows, n_cols) = map(int, stdscr.getmaxyx()) ...doesn't work
+        #    ...map(int, os.popen('stty size', 'r').read().split() ...may work
+        # and then (2) implement the logic to resize all the subwindows as above
+


I tend to not use map/filter and instead just use comprehensions such as [int(v) for v in stdscr.getmaxyx()]. It is more flexible, doesn't require a function (or lambda) definition and call to do things, combines both map and filter functionality, and more. But sure, some people like the 'functional' feel of map etc.

Addressed here; will keep in mind in the future as well, thanks.

altendky · 2021-02-07T00:22:15Z

+        full_refresh = False
        if (elapsed < refresh_period):


Suggested change

full_refresh = False

if (elapsed < refresh_period):

full_refresh = elapsed >= refresh_period

if not full_refresh:

Maybe even rename it do_full_refresh or refresh_needed or...

Makes sense.

altendky · 2021-02-07T00:22:32Z

            jobs = Job.get_running_jobs_w_cache(dir_cfg['log'], jobs)

        else:
+            full_refresh = True


Suggested change

full_refresh = True

altendky · 2021-02-07T00:34:48Z

+        timestamp = datetime.datetime.now().strftime("%H:%M:%S")
+        refresh_msg = "now" if full_refresh else ('%ds/%ds' % (elapsed, refresh_period))
+        header_win.addnstr(' %s (refresh %s)' % (timestamp, refresh_msg), linecap)


I would personally reach for Pendulum pretty quickly when I hit time stuff. Maybe not for these lines but... pretty quickly.

Suggested change

timestamp = datetime.datetime.now().strftime("%H:%M:%S")

refresh_msg = "now" if full_refresh else ('%ds/%ds' % (elapsed, refresh_period))

header_win.addnstr(' %s (refresh %s)' % (timestamp, refresh_msg), linecap)

timestamp = datetime.datetime.now().strftime("%H:%M:%S")

refresh_msg = "now" if full_refresh else f"{elapsed}s/{refresh_period}s"

header_win.addnstr(f" {timestamp} (refresh {refresh_msg})", linecap)

altendky · 2021-02-07T00:39:49Z

+        return (len(cmdline) >= 4 and
+            'python' in cmdline[0] and
+            'venv/bin/chia' in cmdline[1] and
+            'plots' == cmdline[2] and
+            'create' == cmdline[3])


Suggested change

return (len(cmdline) >= 4 and

'python' in cmdline[0] and

'venv/bin/chia' in cmdline[1] and

'plots' == cmdline[2] and

'create' == cmdline[3])

return (

len(cmdline) >= 4

and "python" in cmdline[0]

and "venv/bin/chia" in cmdline[1]

and "plots" == cmdline[2]

and "create" == cmdline[3]

)

Per both my preference and black's. Locating the delimiting bit at the beginning of the line makes it stand out better. Putting all lines inside the parentheses makes them all a bit more equally treated.

This is also basically an @staticmethod. May as well just be a function outside the class.

https://mail.python.org/pipermail/python-ideas/2016-July/041189.html

Honestly, staticmethod was something of a mistake -- I was trying to
do something like Java class methods but once it was released I found
what was really needed was classmethod. But it was too late to get rid
of staticmethod.

Nice, I'm learning a lot.

altendky · 2021-02-07T00:56:10Z

+
+
+def n_at_ph(jobs, ph):
+    return len([j for j in jobs if j.progress() == ph])


I dunno, perhaps this is 'tricky'. But, creating an actual list to count stuff isn't pretty either.

Suggested change

return len([j for j in jobs if j.progress() == ph])

return sum(1 for j in jobs if j.progress() == ph)

That's clever. I was looking for something like "count()" that would take a bool-returning function, kind of like map, but couldn't find one. This works.

altendky · 2021-02-07T01:03:08Z

+def n_to_char(n):
+    if n == 0:
+        return ' '
+    elif n == 1:
+        return '.'
+    elif n == 2:
+        return ':'
+    elif n == 3:
+        return ';'
+    elif n >= 4:
+        return '!'
+    else:
+        return 'X'  # Should never be negative


Suggested change

def n_to_char(n):

if n == 0:

return ' '

elif n == 1:

return '.'

elif n == 2:

return ':'

elif n == 3:

return ';'

elif n >= 4:

return '!'

else:

return 'X' # Should never be negative

n_to_char_mapping = dict(enumerate(" .:;"))

def n_to_char(n):

if n < 0:

return 'X' # Should never be negative

return n_to_char_mapping.get(n, "!")

Might also like mapping to braille dots. I feel a little inappropriate misusing those characters, but they do work nicely for 'modern' spinners and such.

Nice. I took a slightly different approach to this to make the "!" treated more like the other chars.

Braille dots is an interesting option I hadn't thought of. I did figure there would be some extended charset characters I could use but wasn't sure if I wanted to step out of ASCII.

altendky · 2021-02-07T01:09:50Z

@@ -0,0 +1,61 @@
+#!/usr/bin/python3
+
+from unittest.mock import patch


I very rarely mock.

As in, you rarely need to, or there's another mechanism you prefer, or you structure your code so you don't need mock-style mechanisms?

I try to avoid the need. I see tests as a second use of your code. If it's hard to test, it can mean your code isn't as flexible as it ought to be. So yeah, different code structure often. Lots of details to discuss in each actual case.

altendky · 2021-02-07T01:10:00Z

+
+import os
+import pyfakefs
+import unittest


I always use pytest.

Hmm, OK, added an issue to consider switching to pytest.

altendky · 2021-02-07T01:11:15Z

+        return (' '.join(['%d:%d' % pair for pair in phases[:n_first]]) +
+            " [+%d] " % n_elided +
+            ' '.join(['%d:%d' % pair for pair in phases[n_first + n_elided:]]))


More variables, less one-liner here. :]

altendky · 2021-02-07T01:47:14Z

If you want to be picky about your commit messages... GitHub suggests a length limit of 50.

If you want focused granular commits, also and and etc are hints you aren't achieving the goal.

As far as granular PRs... at various points I've made compound PRs because there's a lot of stuff mixing together. When I'm being good, I go back and break off auxiliary PRs. As each of the pieces gets merged and you catch up the original branch, it'll get more and more focused.

ericaltendorf · 2021-02-07T22:36:18Z

Thanks for all the comments. Addressed most, will leave some larger ones (consider use of attrs, pytest) to later.

ericaltendorf added 6 commits February 6, 2021 13:12

changed: rendering of phase lists and certain headers, also added uni…

e7fdb64

…t test

feat: job progress histogram library

e34ea11

feat: add job progress histogram to interactive mode; other minor imp…

5ba4f0b

…rovements to interactive mode display

fix: inspect process cmdline instead of process name when looking for…

6e4474d

… plot jobs

fix: missing 'Job.' prefix

67b769a

fix: add missing parameters in config, improve comments

f9b0adf

ericaltendorf requested a review from altendky February 6, 2021 23:54

altendky requested changes Feb 7, 2021

View reviewed changes

fix: address comments on PR

66f7158

ericaltendorf linked an issue Feb 7, 2021 that may be closed by this pull request

Check in a current and canonicalized config.yaml #7

Closed

ericaltendorf merged commit e9c32a2 into development Feb 8, 2021



		def n_at_ph(jobs, ph):
		return len([j for j in jobs if j.progress() == ph])

	return len([j for j in jobs if j.progress() == ph])
	return sum(1 for j in jobs if j.progress() == ph)

		@@ -0,0 +1,61 @@
		#!/usr/bin/python3

		from unittest.mock import patch

Conversation

ericaltendorf commented Feb 6, 2021

Uh oh!

altendky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

altendky commented Feb 7, 2021

Uh oh!

ericaltendorf commented Feb 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants