Tooling foundation: packaging config, community files, bug fixes & gitignore#59
Tooling foundation: packaging config, community files, bug fixes & gitignore#59emapuljak wants to merge 23 commits into
Conversation
There was a problem hiding this comment.
Hi @emapuljak , thanks a lot for your PR. I really appreciate the effort you've put into it.
-
I would recommend splitting it into two separate PRs: one focused on the CI/CD pipeline, and another for the smaller code improvements. That will make the review process much easier.
-
Also, since the codebase is changing quite frequently, updating 74 modules at once might not be the best approach. It would likely mean rebasing every time Roope makes changes, which could become quite time-consuming.
-
A few of the suggested changes also aren't really needed for this project. I've left more detailed comments in the review.
If anything is unclear or you'd like to discuss any of the feedback, feel free to reach out anytime:)
| "def get_scheduler(optimizer, config):\n", | ||
| " if config.training_parameters.lr_schedule is None:\n", | ||
| " return None\n", | ||
| " elif config.training_parameters.lr_schedule == \"cosine\":\n", |
There was a problem hiding this comment.
it's not necessary to modify elif -> else conditions as there is a return statement;
just a matter of preferences
| "loss_func = nn.CrossEntropyLoss()\n", | ||
| "\n", | ||
| "# Run optimization loop\n", | ||
| "best_params = tuner.run_optimization(model,\n", |
There was a problem hiding this comment.
here, there is a matter of preferences as well, not a strict requirement
| beta = to_numpy(keras.ops.cast(layer.beta, layer.dtype)) | ||
| else: | ||
| beta = np.zeros_like(mean) | ||
| gamma = to_numpy(keras.ops.cast(layer.gamma, layer.dtype)) if layer.scale else np.ones_like(mean) |
There was a problem hiding this comment.
the matter of preferences, not really needed as well
| self.t_start_collecting_batch = self.config.pruning_parameters.t_start_collecting_batch | ||
|
|
||
| def build(self, input_shape): | ||
| self.shape = (input_shape[0], 1) |
There was a problem hiding this comment.
do not see a need in changing this
|
|
||
| def build(self, input_shape): | ||
| self.shape = (input_shape[0], 1) | ||
| shape: tuple[Any, ...] = (input_shape[0], 1) |
There was a problem hiding this comment.
also, specifying the type annotation here is completely optional
There was a problem hiding this comment.
Its a good coding practice, not my preference. So I am just adding ruff/mypy rules that are good for code health.
There was a problem hiding this comment.
Thanks for the comment. The project has been actively developed by @nroope and me for more than two years. Before proposing changes such as switching to Ruff or adding MyPy, it would be helpful to first discuss the needs and priorities of the main contributors and whether those changes would provide meaningful benefits for this project.
Our current pre-commit setup, can be seen in pre-commit-config.yaml file, includes Black, isort, Flake8, and the other tools, that has worked well in practice and satisfies the project's requirements. We're happy with this workflow, and at the moment we don't see a huge need to replace it with newer approaches simply because it's more modern. Given the project's current priorities, I believe there are more impactful tasks to focus on.
There was a problem hiding this comment.
So ruff is explicitly designed to replace Black, isort, Flake8 and many common Flake8 plugins. Thats why I suggested to change it, because it can cover them all. Plus Mypy then adds additional layer that the current tools do not provide - it can catch invalid function arguments, return values, missing attributes, etc. Ruff is released in 2022, opposed to others which are older, and already has the same count of users because its easier to keep all checkes in one library with ruff check and ruff format commands. so basically you can put formatting, import sorting and linting into one tool now.
I agree that maybe I overshot by including them all in one PR. I could have added ruff (replacing these 3), then mypy, then bandit for security checks (which doesnt exist) all in 3 separate PRs. I could still do that. You can also do your research on why ruff is good and what are mypy and bandit.
|
|
||
|
|
||
| class PQLayerNorm(nn.LayerNorm): | ||
| _weight: nn.Parameter | None |
| self.in_quant_granularity = in_quant_granularity | ||
| self.out_quant_granularity = out_quant_granularity | ||
| self.param_quant_granularity = param_quant_granularity | ||
| proj_kwargs = dict( |
There was a problem hiding this comment.
same comment about the matter of preferences
| quantize_input=quantize_input, | ||
| quantize_output=quantize_output, | ||
| ) | ||
| ln_kwargs = { |
There was a problem hiding this comment.
same comment here
|
|
||
|
|
||
| class ActivationPruning(nn.Module): | ||
| mask: Tensor |
There was a problem hiding this comment.
there is no real need to add this type annotation here
| if self.precompute_layer_inputs: | ||
| tmpdir = tempfile.TemporaryDirectory(prefix="ldistil_", dir=self.cache_dir or os.getcwd()) | ||
| dataloader = self.precompute_layer_io(teacher_layer, dataloader, self.device, tmpdir.name) | ||
| tmpdir = tempfile.TemporaryDirectory(prefix="ldistil_", dir=self.cache_dir or str(Path.cwd())) |
There was a problem hiding this comment.
same comment about Path
|
This is one of 4 PRs that need to be done. This does not include CI pipeline at all. Its impossible to have smaller changes then this for adding the tooling because adding it means a lot of rules are added that are changing the code writing, that is why I separated it into 4 PRs. This is just the first one. Also, many comments that you made, are more on code health side, that are imposed by mypy / ruff / bandit and were being addressed. Not my preferences. But good quality of code writing.
All these are added for the library to have a good base for future development which will involve more people/contributors. Some stricter rules on code quality should be in place to avoid any confusion. And when a new feature is being developed that any developer of the library can come and say "I can work on this issue" and you can guarantee that the code would be written in the same manner. It takes some time to get used to these practices but they really make sense overall. And pay off. |
|
If something is more modern and used more then it should be included in my opinion. I am trying to help you stay in track with the current development design, and I already discussed this with Roope and he agreed. If you want to scale this library to something bigger I am giving you suggestions to do so. Don't need to take them. But it will help you in the future. I am not trying to make criticism for the sake of changing something, I am trying to transfer knowledge that other ppl who know better trassfered to me before. |
Closes #54
First of three PRs (A → B → C) porting the
python-templatetooling into PQuantML.What's in it
pyproject.toml: Ruff (line-length 125), mypy, bandit,pytest and coverage sections;
pytest-covintest; newdocsanddevextras;packages.findauto-discovery +package-dataso subpackages andconfigs/*.yamlactually ship; dropped the unused
allextra.CONTRIBUTING.mdandCHANGELOG.md, surfaced in the docs via MyST include - created here so PR B's changelog gate can be added in CIhas something to check.
tox.iniextras name,.readthedocs.yamlconf path,docs/MakefileHGQ leftover, README links, stray root__init__.py, hardcoded docsversion, and
torch.load(..., weights_only=True).KERAS_BACKENDrequirement (previously undocumented —PyTorch users silently got the tensorflow backend), a quick-start example, and
Contributing/License sections.
.gitignore: tool caches + coverage artifacts; narrowed the blanket*.txt.mypy src testsgoes from 256 errors → 0, fixing several realdefects it surfaced — hardcoded CUDA in
add_compression_layers(the library wasunusable on CPU-only machines),
pAdam/pSGD*argscollisions,PQActivation.get_config()calling a nonexistent superclass method, and a missing
update_mask()on the torch PDP.Notes
D(docstring) rule is deliberately excluded — enabling it before thedocstring backfill would flood
ruff check. It goes on in PR C.coverage fail_underis 40 (current measured combined coverage is 43%); raising ittoward 80 is tracked separately - Increase code test coverage to 80% #57
failures are pre-existing and unrelated - will be fixed in Fix tests and code before CI implementation #58