Bringing in updates #15
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR does a couple of things to clean up the boilerplate related to packaging OMLT, see sections below for detailed explanations of the changes. * Remove `setup.cfg` , `setup.py`, `docs/requirements.txt`, `tox.ini` in favour of `pyproject.toml`. * Place `conda` requirements into `environment.yml` * Create new workflows `tests.yml` and `publish_release.yml` * Add quality checks using `ruff`, `mypy`, `doctest` * Use `just` for developer experience * Updated the `Development` section of `README` to talk about `just` * Clean up `conf.py` * Move `pull_request_template.md` * Allow publishing of package to pypi by pushing a new version tag # Other comments * consider internal package structure * force squash merge of PRs - this keeps git history for the `main` branch nice and clean # Using `pyproject.toml` `pyrpoject.toml` is the simplest way to provide package metadata for a Python package. It is easy to read and also provides sections for configurating tools such as `pytest`, `ruff` and `mypy` all in one place. It works seamlessly with the modern Python ecosystem. I set up `pyproject.toml` to automactically detect the version of the code from git tags. No need to duplicate version numbers across the repo. Just add a new tag and everything will be updated. In addition, when a new git tag is pushed to the GitHub repo, the new `publish_release` workflow will be triggered and a new PYPI version released. (See more on this below). I also set it up so that the version is automatically added to a file called `src/omlt/_version.py` which holds the `__version__` variable. this file is autogenerated and therefore added to `.gitignore`. The `__version__` veriable is then re-exported in `src/omlt/__init__.py` so that our users have access to it. I tried to perserve all the information stored in the `setup.cfg` and other deleted files -- let me know if there is something i missed! ## Optional dependencies The `pyproject.toml` file allows the creation of optional dependencies. For example, our users can install ```bash pip install omlt[keras] # or pip install omlt[torch] # or pip install omlt[linear-tree,keras-gpu] ``` Ofc any combination of optional dependencies is valid too. This allows our users to install the dependencies specific to their use case. Note that: * I made `onnx` and `onnxruntime` a required dependency because from my understanding it is almost always used * I added an optinoal dependency set called `dev` which developers can use to install all developer tools and all dependencies -- you need this to run all the tests for example * There is also `dev-gpu` which installs the GPU version of tensorflow in case the developer has a GPU The available optional dependencies are: * `linear-tree`, installs the linear tree dependency * `keras`, installs tensorflow and keras * `keras-gpu`, installs tensorflow for the gpu and keras * `torch`, installs torch and torch geometric * `dev-tools` - this is not to be used directly but allows easy re-use of dev tools in other optional dependencies, namely dev and dev-gpu * `docs` - installs dependencies required to compile docs * `dev` - dependecies needed for developing the project, such tooling * `dev-gpu` - same as dev but installed with gpu support Our documentation probably needs to be updated to tell users they wanna install omlt with some combination of `linear-tree`, `keras`, `keras-gpu`, `torch` optional dependencies depending on what features of the package they are using # Quality checks with `ruff`, `mypy` and `doctest` I've enabled `ruff`, `mypy` and `doctest`. Currently there are no doctests, but its good to have it set up so that it runs in case any are added in the future. Both `ruff` and `mypy` are failing because there are a number of things which need to fixed. For both `ruff` and `mypy` I have disabled some checks which it would be good to enable eventually but are probably a fair amount of work to fix -- these have comments in `pyproject.toml`. The remaining failing checks are ones which I would reccomend fixing ASAP. There's two approaches, merge now and fix these errors later. Or keep a separate branch where these are incrementally fixed. Up to you to decide what you prefer. I told ruff to check for `google` style docstrings. I think these are the best because they have good readbility and work the best with type hints in my opinion. # Using `just` instead of `tox` https://github.com/casey/just is a simple command runner. It allows the developers to define and re-use common operations, for example I can define a `check` recipe and then run ```bash just check ``` in my command line and it will run all the tests. The beauty of this is that `just` is extremely simple. If you read the file its basically a sequence of bash instructions for each recipe. This makes the `recipes` really transparent, and easy to understand, and works as code-as-documentation. Users can just read the recipe and run the commands one by one to get the same effect without having `just` installed. There is no magic which helps with debugging issues. It's also language agnostic. `just` comes as a small stand-alone binary, which makes it a very non-intrusive tool to have on your computer that does not need any dependencies. The downside is that it does not provide automatic management for Python environments, which I belive tox does provide. The other side of this is that we allow developers to use their favorite tools for managing venvs rather than proscribing certain tools for this repo. (the difference with `just` being that it is essentially optional tool and also serving as documentation) I may be overly opinionated on this one, so feel free to push back. # Cleaning up `docs/conf.py` I removed a bunch of the commented out code. This makes it easier to see what the configuration is and also prevents the commented out options from becoming out of date when a new release of sphinx is made. # Moving `pull_request_template.md` I moved this into the `.github` folder because it is GitHub configuration. Very optional, but makes more sense to me. # `readthedocs` automated action this guide https://docs.readthedocs.io/en/stable/guides/pull-requests.html shows how to set it up. requires admin permissions on readthedocs -- can jump on a call to help with this # publishing with to `PYPI` with a git tag for this an API key for PYPI needs to be created and added to the repos secrets -- can jump on a call to help with this # consider `_internal` package structure One way to make it easier to manage private vs public code in a repository is to create an `_internal` folder where all the code goes. This way all code can be shared easily and moved between modules and its by default private, so changes to internal code does not break users. Public modules then just re-export code in the `_internal` submodules. You can see an example of this structure here https://github.com/lukasturcani/stk. Not a huge issue but I find it very helpful for managing what things are actually exposed to users the code-base grows. **Legal Acknowledgement**\ By contributing to this software project, I agree my contributions are submitted under the BSD license. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer. --------- Co-authored-by: Jeremy Sadler <[email protected]>
…ons (cog-imperial#163) This PR adds a tolerance at which to enforce ``strict'' inequalities in linear model trees: That is, the right branch will require that the feature value be greater than or equal to the bound plus this tolerance (epsilon). This means that users can tune epsilon in order to ensure that the MIP solution will match the tree prediction. Additionally, the PR simplifies the implementation of the hybrid bigm linear tree formulation by using two modern pyomo.gdp transformations. This does mean that the linear tree formulations will rely on pyomo>=6.7.1 though, if that's okay. **Legal Acknowledgement**\ By contributing to this software project, I agree my contributions are submitted under the BSD license. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer. --------- Co-authored-by: Emma Johnson <[email protected]>
Pyomo recently made ComponentData classes public (Pyomo/pyomo#3221) which will be part of the upcoming release. Currently, this causes the following error to occur in OMLT: ``` TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases ``` The Pyomo team is working to try to address this issue, however OMLT should update its code to address this as otherwise deprecation warnings will be emitted when using the old class names. The fix is to replace all instances of `_BlockData` with `BlockData` (just removing the underscore) - this applies to any other instance of Pyomo component data objects as well (although I could only find 2 instances of these in the OMLT code). **Legal Acknowledgement**\ By contributing to this software project, I agree my contributions are submitted under the BSD license. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer. Co-authored-by: jalving <[email protected]>
…erial#143) I assume that the notebooks have been moved, but the documentation links did not reflect that **Legal Acknowledgement**\ By contributing to this software project, I agree my contributions are submitted under the BSD license. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.
copying CI workflow over
…hout the codebase.
…ated variables and constraints
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Legal Acknowledgement
By contributing to this software project, I agree my contributions are submitted under the BSD license.
I represent I am authorized to make the contributions and grant the license.
If my employer has rights to intellectual property that includes these contributions,
I represent that I have received permission to make contributions and grant the required license on behalf of that employer.