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

Eliminate garage.misc #358

Closed
ryanjulian opened this issue Oct 25, 2018 · 4 comments
Closed

Eliminate garage.misc #358

ryanjulian opened this issue Oct 25, 2018 · 4 comments

Comments

@ryanjulian
Copy link
Member

garage.misc is a dumping ground for code which should mostly be organized somewhere else. There's also a huge list of "utils" modules hiding around everywhere, many duplicating code in the others.

Here we will audit all the code in garage.misc to determined whether it should be

  1. Deleted, because it's terrifying (e.g. stub) or badly-designed
  2. Replaced by a library dependency
  3. Moved to its own top-level module or into some existing module (e.g. garage.logger)
  4. Begrudgingly accepted for now, and generate an issue to rewrite it
  5. Left alone

We will algo audit all the utils modules to have as few as possible.

@ryanjulian ryanjulian added this to the Sprint 7 milestone Oct 26, 2018
@ryanjulian ryanjulian added the quality Code quality enhancements label Oct 26, 2018
@naeioi
Copy link
Member

naeioi commented Oct 29, 2018

@ryanjulian Any suggestion of people I can talk to on stub()? I have no clear idea of what it is used for right now.

@ryanjulian
Copy link
Member Author

This is distinct from stub(). I will comment on that issue #239

@ryanjulian ryanjulian modified the milestone: Sprint 7 Oct 30, 2018
@ryanjulian
Copy link
Member Author

ryanjulian commented Nov 3, 2018

Audit of garage.misc

Module Description Verdict Destination Module PRs
autoargs Decorator for injecting command line arguments directly into constructors 🛑 Remove N/A #573
console Console and filesystem utilities, plus some meta-programming nonsense 🚧 Refactor garage.logger #464
ext ? ? ?
instrument Actually the experiment runner 🚧 Refactor garage.experiment #386
krylov ? ? ?
logger API for the logging facility 🚧 Refactor garage.logger #464
mako_utils Snippets for mako models 🚧 Refactor rlworkgroup/metaworlds #488
meta Literally empy 🛑 Remove N/A #492
nb_utils Tools for processing experiments with Jupyter notebook 🚧 Refactor garage.experiment.nb_utils #493
overrides Decorator for annotating and verifying overided methods 📚 Replace with library N/A
resolve Class resolution tools Remove rlworkgroup/metaworlds #488
special ? ? ?
tabulate Builds tables for printing to the console 📚 Replace with library N/A #464
tensor_utils ? ? ?
tensorboard_output TensorBoard sink for logging 🚧 Refactor garage.logger #419
viewer_2d General-purpose 2D image-based plotter in PyGame 🛑 Remove N/A #492
prog_bar_counter.py Paints progress bars to stdout 🚧 Refactor garage.logger

Notes

autoargs

"Command line arguments anywhere" is a tempting pattern, but IME this often escalates into entry points with giant numbers of irrelevant, impossible-to-understand command line args. Unless you have a very special application architecture, it's probably better for libraries to be configured in code and not on the command line.

I prefer if we force people to specify their command line args only at the entry point, the old-fashioned away. autoargs injects arbitrary new arguments into __init__() without forcing you to modify the callsite, making constructors hard to read. If we wanted functionality like this we could probably find it in any number of Python command line arguments libraries.

console

This appears to contain mostly utilities used by the logger, plus some odds and ends. It includes raw console printing, colorizing of log messages, mkdir -p in Python, data structures for logs, and a utility for querying Y/N from the user.

Almost all of this could be replaced by a library or moved into the logger module.

Console printing

Use logging from stdlib or

Logger data structures

Possibly a part of a future logger

Colorization

https://github.com/tartley/colorama
https://github.com/magmax/colorize
https://github.com/dslackw/colored

Meta-programming nonsense

Get rid of it

mkdir -p

AFAICT this is now implemented in the Python 3 stdlib and we no longer need our own implementation. It may have been missing in Python 2.

logger

API for the logging facility. This is more bespoke than it needs to be, and a lot of the console logging can be replaced with the Python logger. This module also handles checkpointing. TBD whether we should split checkpointing into its own module.

overrides

This is a method decorator for import-time checking that a method which claims to be overriding a superclass method is actually doing that. It is slightly less-relevant if we refactor to make good use of ABCs, but I think still useful to have.

The current copy is a straight copy/paste of a pip library with some code commented out. See: https://github.com/mkorpela/overrides

@ryanjulian
Copy link
Member Author

ryanjulian commented Mar 7, 2019

Updated view

Module Description Verdict Destination Module PRs
console Console and filesystem utilities, plus some meta-programming nonsense 🚧 Refactor ???
ext Random seed plus various utilities 🚧 Refactor garage.experiment, garage.tf.optimizers #578
krylov Math utils for an optimization procedure ??? garage.math?
overrides Decorator for annotating and verifying overided methods 📚 Replace with library N/A
special Lots of math utilities ??? ?
tensor_utils More math utilities, mostly tensor manipulation ? ?
prog_bar_counter.py Paints progress bars to stdout 📚 Replace with library click

ryanjulian added a commit that referenced this issue Mar 7, 2019
@ryanjulian ryanjulian moved this from To do to In progress in Code Audit Mar 7, 2019
@ryanjulian ryanjulian moved this from In progress to Done in Code Audit Nov 5, 2019
@ryanjulian ryanjulian added this to the v2020.02rc3 milestone Nov 6, 2019
@ryanjulian ryanjulian added housecleaning and removed quality Code quality enhancements labels Nov 19, 2019
@ryanjulian ryanjulian removed this from the v2020.02rc3 milestone Feb 27, 2020
@ryanjulian ryanjulian modified the milestones: v2020.06rc1, v2020.06.0 Feb 27, 2020
@ryanjulian ryanjulian modified the milestones: v2020.06rc1, v2020.06.0 Mar 29, 2020
@ryanjulian ryanjulian removed this from the v2020.09.0 milestone May 27, 2020
@ryanjulian ryanjulian changed the title Audit garage.misc and all utils modules Eliminate garage.misc May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Code Audit
  
Done
Development

No branches or pull requests

3 participants