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

High priority best practices #96

Merged
merged 20 commits into from
Feb 21, 2023
Merged

Conversation

daymxn
Copy link
Contributor

@daymxn daymxn commented Feb 13, 2023

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:

  • 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
  • When checking for None obj is [not] None should be used instead of type (as above) or ==
  • 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.
  • 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:

  • 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
Copy link
Owner

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

@daymxn
Copy link
Contributor Author

daymxn commented Feb 15, 2023

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
Copy link
Contributor Author

daymxn commented Feb 18, 2023

Friendly ping @manny405

@manny405
Copy link
Owner

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

Copy link
Owner

@manny405 manny405 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@manny405 manny405 merged commit ae9dde3 into manny405:main Feb 21, 2023
@daymxn daymxn deleted the daymon-best-practices branch February 21, 2023 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants