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

Remove redundant third party dependencies. #15

Merged
merged 11 commits into from Apr 6, 2021

Conversation

johnamcleod
Copy link
Contributor

No description provided.

pvrancx
pvrancx previously approved these changes Mar 30, 2021
Copy link

@pvrancx pvrancx left a comment

Choose a reason for hiding this comment

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

assuming install still works - LGTM ;-)

pyproject.toml Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #15 (927e4ed) into develop (2f3f736) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop      #15   +/-   ##
========================================
  Coverage    92.40%   92.40%           
========================================
  Files           66       66           
  Lines         1949     1949           
========================================
  Hits          1801     1801           
  Misses         148      148           

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 2f3f736...927e4ed. Read the comment docs.

Copy link
Contributor

@hstojic hstojic left a comment

Choose a reason for hiding this comment

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

can we discuss first that removal of docs dependencies from development section of pyroject.toml file?

@hstojic hstojic requested a review from pvrancx March 30, 2021 13:26
@pvrancx pvrancx dismissed their stale review March 30, 2021 14:00

more discussion needed

@pvrancx
Copy link

pvrancx commented Mar 30, 2021

can we discuss first that removal of docs dependencies from development section of pyroject.toml file?

I'm ok with docs having a separate build process, provided there is an automated command and it is documented.
I could see this being a part of the dev install, but don't feel strongly about this. TBH I tend not to build docs and just consult them online.

@johnamcleod
Copy link
Contributor Author

can we discuss first that removal of docs dependencies from development section of pyroject.toml file?

I'm ok with docs having a separate build process, provided there is an automated command and it is documented.
I could see this being a part of the dev install, but don't feel strongly about this. TBH I tend not to build docs and just consult them online.

@hstojic is right - I had forgotten that there is a poetry command to build the docs, which at the moment does not install anything. There are three ways to address this:

  1. update the command which builds the docs to install the extra requirements
  2. Restore the dev dependencies for building the docs
  3. Restore the previous action definition which explicitly installed the extra requirements

I would prefer the keep the dev dependencies and doc dependencies separate, so I would prefer 1) or 3), but I do not feel strongly about this.

@pvrancx
Copy link

pvrancx commented Mar 30, 2021

  • update the command which builds the docs to install the extra requirements
  • Restore the dev dependencies for building the docs
  • Restore the previous action definition which explicitly installed the extra requirements

So 1) would enable a separate poetry command that installs the dependencies and builds the docs? That would be my preference. I'd prefer to minimize the dependencies you are forced to install. Having a separate command gives maximum control to the user.

@hstojic
Copy link
Contributor

hstojic commented Mar 30, 2021

not sure I understand what is 3)...

what about 4) where in dev part of dependencies we specify docs related packages as optional (same as for mujoco-py), for those contributors that might work on docs and will need/want to check whether it builds?

that would be better than 1) which would install packages every time I build docs (which might happen a lot when you are actively working on them)

depending on what we implement, we should update CONTRIBUTING.md, where it talks about building docs...

@johnamcleod
Copy link
Contributor Author

what about 4) where in dev part of dependencies we specify docs related packages as optional (same as for mujoco-py), for those contributors that might work on docs and will need/want to check whether it builds?

That's good - I will go with that.

depending on what we implement, we should update CONTRIBUTING.md, where it talks about building docs...

Good point - I will also update this.

@hstojic
Copy link
Contributor

hstojic commented Apr 2, 2021

  1. doesn't seem to work, just tried it

it seems that optional flag cannot be used in dev-dependencies block: https://stackoverflow.com/questions/60971502/python-poetry-how-to-install-optional-dependencies#60990574

I think I would then go with 2), only contributors will be installing these few additional libs, and if they are using poetry then they are installing them into a virtual envirionment and there shouldnt be any issue with installing these few libs for the project...

@johnamcleod johnamcleod changed the base branch from develop to hstojic/readthedocs April 6, 2021 13:51
@johnamcleod johnamcleod changed the base branch from hstojic/readthedocs to develop April 6, 2021 13:51
@johnamcleod
Copy link
Contributor Author

I have updated the contributors file to describe how to build the docs, and I have verified locally that the instructions work. The documentation build dependencies are listed in the docs_requirements.txt file, which can be installed separately from the dev dependencies to build the docs.

Copy link

@pvrancx pvrancx left a comment

Choose a reason for hiding this comment

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

Looks fine to me - does this work for you @hstojic ?

@johnamcleod johnamcleod merged commit da42a2e into develop Apr 6, 2021
@johnamcleod johnamcleod deleted the john/curate_dependencies branch April 6, 2021 21:14
hstojic added a commit that referenced this pull request Apr 7, 2021
* Framework (#7)

* Decision planning agents. (#8)

* Background planning agents. (#9)

* implemented coverage report with codecov.io (#11)

* draft for codecov

* added my PR branch

* removing files spec

* corrected an error in specifying file names

* updated readme with badges and workflow badges

* removed pypi installation instruction (#10)

Co-authored-by: hstojic <hrvoje.stojic@protonmail.com>

* Hstojic/trigger docs (#12)

* modified publish-docs to trigger docs generation at website repo

* tag example

* accessing tag/branch environment variable

* additional variable

* removed testing branch

* updating readme files - slack info, citaiton (#13)

* setting things up for pypi (#14)

* setting things up for pypi

* isort; using main instead of version tags

* corrected setup file; added MANIFEST file

* updated readme file with pypi stuff

* black

* Remove redundant third party dependencies. (#15)

* corrected workflows; small updates to the readme; corrected docs version

* automatized release tag

Co-authored-by: John Mcleod <43960404+johnamcleod@users.noreply.github.com>
Co-authored-by: Felix Leibfried <felix.leibfried@gmail.com>
Co-authored-by: Hrvoje Stojic <hrvoje.stojic@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants