Closed julianstirling closed 6 months ago
Hi @jmwright, it is now 3am and I have been working on this for 17 hours straight. The code is far from perfect but it is now much more modular and parametric, with a few more doc_strings and comments. But they are still pretty bare bones.
ShelfBuilder is somewhat de-tangled to remove the huge number of instance attributes, some defined well after initialisation. These are now generally derived properties. I also force the main properties to be shared at initialisation and build the front on initialisation. This is needed because otherwise if you ran other methods before the front is created you will get errors.
I have removed the old nimble_tray.py and instead there is a generic tray in tray_6in.py I have nominally updated the server. But not in a way that is tested (I left a note about this as some the variable to pass have now changed.)
There is still a fair bit of work to do to properly parametrise the code base. I have captured some of that in this issue. There is probably more, but I now need to sleep!!
@julianstirling Thanks for all the work on this. It is a lot to take in, but I did my best to review it.
I have peppered suggestion comments throughout. Many of these are related to minor things in docstrings. We do not need to burn time discussing these, just mark as resolved without merging the suggestion if you don't want it.
I also made a few comments on higher level questions with the code and design.
When I run python generate.py
(on this branch) per the instructions, I get the following error.
Starting build
Generating components
INFO:exsource:Processing export: rack_leg
Traceback (most recent call last):
File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/exsource_tools/exporters.py", line 164, in run_executable
ret = subprocess.run(args, check=True, capture_output=True)
File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/subprocess.py", line 526, in run
raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['cq-cli', '--params', 'length:116;hole_spacing:14;', '--infile', '../mechanical/components/cadquery/rack_leg.py', '--outfile', './printed_components/beam.step']' returned non-zero exit status 100.
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "/home/jwright/repos/nimble/generate.py", line 38, in <module>
generate_example_configuration()
File "/home/jwright/repos/nimble/generate.py", line 35, in generate_example_configuration
generate(selected_devices_ids)
File "/home/jwright/repos/nimble/generate.py", line 25, in generate
runner.generate_components(config.components)
File "/home/jwright/repos/nimble/nimble_orchestration/orchestration.py", line 47, in generate_components
self._run_exsource(exsource_path)
File "/home/jwright/repos/nimble/nimble_orchestration/orchestration.py", line 95, in _run_exsource
processor.make()
File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/exsource_tools/tools.py", line 98, in make
unprocessed_exports = self._process_exports(unprocessed_exports)
File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/exsource_tools/tools.py", line 135, in _process_exports
cqe.process_export(output_file_statuses=self._all_output_files)
File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/exsource_tools/exporters.py", line 186, in process_export
self.run_executable(output, require_x)
File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/exsource_tools/exporters.py", line 169, in run_executable
raise RuntimeError(f"\n\n{self.name} failed create file: {output.filepath}"
RuntimeError:
CadQuery failed create file: ./printed_components/beam.step with error:
Traceback (most recent call last):
File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/cq_cli/main.py", line 38, in build_and_parse
raise (build_result.exception)
File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/cadquery/cqgi.py", line 105, in build
self.set_param_values(build_parameters)
File "/home/jwright/mambaforge/envs/nimble/lib/python3.10/site-packages/cadquery/cqgi.py", line 137, in set_param_values
raise InvalidParameterError(
cadquery.cqgi.InvalidParameterError: Cannot set value 'hole_spacing': not a parameter of the model.
Thanks @jmwright I'll get your changes merged in and check to see if I can replicate that error. Will probably be Monday as I'm on the road this weekend.
@jmwright The generate.py script should work again. Thanks for catching this. I did run it periodically, but I clearly didn't run it right at the end. We should get that in the CI soon, so we catch these kind of things.
I think I have merged all of your suggestions. For every high level comment that needed an action I have either opened an issue or added it as a task to an issue.
Assuming the code works are you happy for this to be merged? Feel free to merge it if you accept the review
Thanks again
Thanks @julianstirling !
PyLint has detected a number of issues which are now checked as GitHub Actions.
The plan is to get the PyLint rating above 9.75 if possible. No plan to aim for fixing every issue. For example some warnings about high numbers of of arguments or instance attributes are easier to ignore when passing around data.