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

refactor(All APIs): Added new pre-commit hook for ordering functions #22830

Closed
wants to merge 3 commits into from
Closed

refactor(All APIs): Added new pre-commit hook for ordering functions #22830

wants to merge 3 commits into from

Conversation

NripeshN
Copy link
Contributor

@NripeshN NripeshN commented Aug 31, 2023

This would mean this ivy-llc/lint-hook#4 is successful

@ivy-leaves ivy-leaves added PaddlePaddle Backend Developing the Paddle Paddle Backend. PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist Ivy Functional API Ivy API Experimental Run CI for testing API experimental/New feature or request labels Aug 31, 2023
@github-actions
Copy link
Contributor

Thanks for contributing to Ivy! 😊👏
Here are some of the important points from our Contributing Guidelines 📝:
1. Feel free to ignore the run_tests (1), run_tests (2), … jobs, and only look at the display_test_results job. 👀 It contains the following two sections:
- Combined Test Results: This shows the results of all the ivy tests that ran on the PR. ✔️
- New Failures Introduced: This lists the tests that are passing on main, but fail on the PR Fork. Please try to make sure that there are no such tests. 💪
2. The lint / Check formatting / check-formatting tests check for the formatting of your code. 📜 If it fails, please check the exact error message in the logs and fix the same. ⚠️🔧
3. Finally, the test-docstrings / run-docstring-tests check for the changes made in docstrings of the functions. This may be skipped, as well. 📚
Happy coding! 🎉👨‍💻

@ivy-leaves
Copy link

If you are working on an open task, please edit the PR description to link to the issue you've created.

For more information, please check ToDo List Issues Guide.

Thank you 🤗

Copy link
Contributor

@vedpatwardhan vedpatwardhan left a comment

Choose a reason for hiding this comment

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

Just came across a few issues while going through the initial files, the same also apply to all files changed. Given that this modifies quite a lot of files, we should take our time and go through all changes made step by step before merging. For a start, could you please try and get the formatter updated according to the suggestions below? Thanks for diving into this @NripeshN, let's see what @KareemMAX thinks 😄

@@ -9,6 +9,26 @@
from ivy.functional.backends.jax import JaxArray
from ivy.functional.ivy.data_type import _handle_nestable_dtype_info

char_rep_dtype_dict = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think char_rep_dtype_dict is better placed after ivy_dtype_dict and native_dtype_dict

@@ -245,6 +180,45 @@ def as_native_dtype(
)


# Array API Standard #
Copy link
Contributor

Choose a reason for hiding this comment

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

The Array API Standard section should be present before the Extra section

@@ -15,54 +15,44 @@
)


# Helpers #
Copy link
Contributor

Choose a reason for hiding this comment

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

Private helpers should always be at the start of the file

@@ -76,6 +66,10 @@ def _to_device(x, device=None):
return x


# --- Main --- #
Copy link
Contributor

Choose a reason for hiding this comment

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

we should rename this section to Extra if it isn't a part of the Array API Standard

def clear_cached_mem_on_dev(device: str, /):
return None


def _dev_is_available(base_dev):
# API #
Copy link
Contributor

Choose a reason for hiding this comment

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

API should instead be Array API Standard

return jnp.where(x != 0, x / jnp.abs(x), 0)


# --- Main --- #
Copy link
Contributor

Choose a reason for hiding this comment

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

Main should be replaced by Array API Standard

@NripeshN
Copy link
Contributor Author

NripeshN commented Aug 31, 2023

Just came across a few issues while going through the initial files, the same also apply to all files changed. Given that this modifies quite a lot of files, we should take our time and go through all changes made step by step before merging. For a start, could you please try and get the formatter updated according to the suggestions below? Thanks for diving into this @NripeshN, let's see what @KareemMAX thinks 😄

@vedpatwardhan, The formator segregates the file into 2 halves. The first half is helpers and the other half would be Main functions. The headers are helpers and main respectively. We could remove headers like # Array API Standard # and # Extras # manually. Adding this functionality to the formatter would result in unnecessary complexity to the formatter and I think doing this task once manually would fix the issue as the formatter adds headers automatically(helpers and main)

@vedpatwardhan
Copy link
Contributor

vedpatwardhan commented Sep 1, 2023

@vedpatwardhan, The formator segregates the file into 2 halves. The first half is helpers and the other half would be Main functions. The headers are helpers and main respectively. We could remove headers like # Array API Standard # and # Extras # manually. Adding this functionality to the formatter would result in unnecessary complexity to the formatter and I think doing this task once manually would fix the issue as the formatter adds headers automatically(helpers and main)

I couldn't fully follow actually, but the "Array API Standard" header and the "Extras" header are important for us to segregate between functions that are and are not present in the standard as new functions not present in the standard get added on a daily basis. Ideally we shouldn't remove these headers, we should leave them unchanged. So in case we want to sort functions alphabetically, I think we should sort them separately for the functions in the Array API Standard and the Extras sections which would make the structure organized while also retaining the array api and extras distinction. And the Array API Standard header is just an example, we also have different section names like Autograd, Optimizer Steps and Optimizer Updates in gradients.py, Array Printing, Retrieval, Conversions, Memory and others in device.py and similarly in other files. The section headers might not be formatted as correctly as they should be (i.e. with the - below the header and so on) but they do convey the information to anyone seeing the code regarding what the functions in that section do, which I think is more beneficial than clubbing all of them into Main API

The overall structure I think should be treated as the template while updating the formatter,

# global
import ...

# local
import ...


# Global declarations
<global variables like FN_CACHE and INF, global mode stacks and their initializers, postponed evaluation typehints, etc.>


# Helpers #
# -------- #

<all private helpers for the submodule>


# Classes

<any classes in the submodule>


# <function section header 1> --> any arbitrary name

<all functions in section 1 sorted alphabetically>
<could also have any assignment statements dependent on whether that function was defined, same for the following sections>


# <function section header 2> --> any arbitrary name

<all functions in section 2 sorted alphabetically>

.
.
.
.

# <function section header n> --> any arbitrary name

<all functions in section n sorted alphabetically>

Happy to know what you think @NripeshN @KareemMAX, thanks 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ivy API Experimental Run CI for testing API experimental/New feature or request Ivy Functional API PaddlePaddle Backend Developing the Paddle Paddle Backend. PyTorch Frontend Developing the PyTorch Frontend, checklist triggered by commenting add_frontend_checklist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants