manny405 / sapai

Super auto pets engine built with reinforment learning training in mind
MIT License
67 stars 21 forks source link

High priority best practices #96

Closed daymxn closed 1 year ago

daymxn commented 1 year ago

Per #90,

This refactors various sites where flow could be updated according to more modernized practices; completing the checklist for high priority best practices (#90), and one of the medium priority tasks. More specifically, this closes the following tasks:

High priority:

  • [x] Replace all occurrences of type(<obj>).__name__ == "<Class>" with isinstance(<Class>, <obj>). This will allow custom subclasses to be made and used with the project in the future
  • [X] When checking for None obj is [not] None should be used instead of type (as above) or ==
  • [X] Default arguments in should never be mutable (e.g. a list or dictionary) as they are not reset between function calls and this can lead to bugs. Instead, set default to None and replace None with the required value in the function.
  • [X] When checking if a value is True or "Truthy" use if value and never if value == True. if value is True should be avoided unless checking for exactly True (will be false if compared to numpy's value of True)

Medium Priority:

  • [X] Use f-strings instead using .format on strings, this makes reading and writing much clearer e.g. replace "pet-{}".format(name) with f"pet-{name}"
manny405 commented 1 year ago

Any idea how to resolve the missing libcublas.so.11 file? Looks issue with environment setup with CI?

daymxn commented 1 year ago

Any idea how to resolve the missing libcublas.so.11 file? Looks issue with environment setup with CI?

Yeah, it seems to stem from CUDA links not matching up between torch and NVIDIA, see pytorch/issues/88882

To work around this, we can migrate to pip and specify the index url in CI. I've updated the workflow to do such :)

daymxn commented 1 year ago

Friendly ping @manny405

manny405 commented 1 year ago

Thanks, I'll review it sometime over the long weekend.