gazebosim / gz-mujoco

25 stars 4 forks source link

Use a single namespace and subpackages #39

Closed j-rivero closed 2 years ago

j-rivero commented 2 years ago

The PR moves the current packages under a common native namespace named sdformat_mjcf.

With the change, we might want to reconsider the way of invoking our imports, probably need to work using the schema from sdformat_mjcf.sdformat_to_mjcf import foo since the installation using PYPI is probably installing the code using that directory layout. By now, things keep working since we are injecting PYTHONPATH in CI to point to the subpackages.

I've manually uploaded a test to test.pypi.org using the initial state of this PR https://test.pypi.org/project/sdformat-mjcf/. I'll send a second PR with the automated code for this.

codecov[bot] commented 2 years ago

Codecov Report

Merging #39 (ed9e471) into main (7f0a306) will increase coverage by 1.12%. The diff coverage is n/a.

:exclamation: Current head ed9e471 differs from pull request most recent head 12231f4. Consider uploading reports for the commit 12231f4 to get more accurate results

@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
+ Coverage   96.94%   98.06%   +1.12%     
==========================================
  Files          11       22      +11     
  Lines         327     1187     +860     
==========================================
+ Hits          317     1164     +847     
- Misses         10       23      +13     
Impacted Files Coverage Δ
...rmat_mjcf/sdformat_to_mjcf/sdformat_to_mjcf/cli.py 90.90% <ø> (ø)
...at_to_mjcf/sdformat_to_mjcf/converters/geometry.py 92.42% <ø> (ø)
...ormat_to_mjcf/sdformat_to_mjcf/converters/joint.py 96.92% <ø> (ø)
...ormat_to_mjcf/sdformat_to_mjcf/converters/light.py 100.00% <ø> (ø)
...format_to_mjcf/sdformat_to_mjcf/converters/link.py 100.00% <ø> (ø)
...at_to_mjcf/sdformat_to_mjcf/converters/material.py 100.00% <ø> (ø)
...ormat_to_mjcf/sdformat_to_mjcf/converters/model.py 100.00% <ø> (ø)
...format_to_mjcf/sdformat_to_mjcf/converters/root.py 100.00% <ø> (ø)
...ormat_to_mjcf/sdformat_to_mjcf/converters/world.py 100.00% <ø> (ø)
...dformat_to_mjcf/sdformat_to_mjcf/sdf_kinematics.py 94.73% <ø> (ø)
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7f0a306...12231f4. Read the comment docs.

j-rivero commented 2 years ago

Right now both sdformat_to_mjcf and sdformat_mjcf_utils have a setup.py file. Do we need that if we're putting them all in one PyPI package?

Oh, that is right, I think we don't need to ship more setup.py than the one in root. I went a bit crazy and do the following in 12231f4:

With this, I'm able to do the following:

> virtualenv foo && source foo/bin/activate && pip install .
> sdformat2mjcf

And python3 setup.py sdist bdist bdist_wheel output also looks good to me.

What I think it is missing:

azeey commented 2 years ago

Okay, but now this puts files in the root directory gz-mujoco. We don't want gz-mujoco to be a python package. If we're putting them all in one package, we should just have sdformat_mjcf as the only directory in the root of the repo and no other files for now.

azeey commented 2 years ago

Replaced by #74