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

Cleaning setup #890

Merged
merged 4 commits into from
Jul 1, 2024
Merged

Cleaning setup #890

merged 4 commits into from
Jul 1, 2024

Conversation

VukW
Copy link
Contributor

@VukW VukW commented Jun 29, 2024

Proposed Changes

  • Right now setup.py places some GANDLF repo files to site-packages/ of the user's python env. This cleaning helps to keep all gandlf files in site-packages/GANDLF so keeping user env tidy + removes other unused parts of setup.py
  • Fix for Adding function to get augmentation transforms #889

@VukW VukW requested a review from a team as a code owner June 29, 2024 13:04
Copy link
Contributor

github-actions bot commented Jun 29, 2024

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

Comment on lines -20 to -34
class CustomInstallCommand(install):
def run(self):
install.run(self)


class CustomDevelopCommand(develop):
def run(self):
develop.run(self)


class CustomEggInfoCommand(egg_info):
def run(self):
egg_info.run(self)


Copy link
Contributor Author

Choose a reason for hiding this comment

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

These commands are just not used (I mean, they does not differ from default ones)

Comment on lines -50 to -52
setup_files = ["setup.py", ".dockerignore", "pyproject.toml", "MANIFEST.in"]
all_extra_files = dockerfiles + setup_files
all_extra_files_pathcorrected = [os.path.join("../", item) for item in all_extra_files]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After installement, user receives this files directly in site-packages/ folder. The reason is using ../ in the path; however, that is caused by the fact these files are located in repo root.

Good practice for building a wheel by setup.py (and, actually, the only possible one) is not to add these files to the wheel. Only files that are required for module should be added. Moreover, any file that should be added to user's site-packages/GANDLF module should be located in module source library also (./GANDLF/ in our case, not in a repo root).

So, there are two options:
(1) move necessary files to ./GANDLF/ module source folder;
(2) just don't include them to the wheel at all.

I chose the second solution.

setup.py Outdated
Comment on lines 35 to 37
toplevel_package_excludes = [
"GANDLF.GANDLF",
"anonymize",
"cli",
"compute",
"data",
"grad_clipping",
"losses",
"metrics",
"models",
"optimizers",
"schedulers",
"utils",
"testing*",
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are not a top-level packages anymore (as they are located inside top-level GANDLF package). However, the testing is a top-level (together with GANDLF); thus we need to exclude it from a wheel (right now a wheel installs a testing module also)

Copy link
Collaborator

@sarthakpati sarthakpati left a comment

Choose a reason for hiding this comment

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

Tests failing.

FAILED testing/test_full.py::test_generic_config_read - TypeError: affine() got an unexpected keyword argument 'degrees'

@VukW
Copy link
Contributor Author

VukW commented Jun 29, 2024

Hi!
Concerning pytest failure - the failed test is actually brought by #889 - new augmentations. Let's proceed there, restored branch & PR

Concerning linter issues - I'll fix it in this PR

@szmazurek
Copy link
Collaborator

Hey!
I agree with your approach @VukW for the setup file. Regarding the augmentations - interesting thing, I did not receive any errors during tests for this pr. Will also take a look, if you find some cause of that earlier please let me know.

@sarthakpati
Copy link
Collaborator

Thanks, folks!

@VukW VukW merged commit 1715c1d into mlcommons:new-apis_v0.1.0-dev Jul 1, 2024
21 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants