Skip to content

Create slddb package#12

Open
bmaranville wants to merge 12 commits intomasterfrom
create-slddb-package
Open

Create slddb package#12
bmaranville wants to merge 12 commits intomasterfrom
create-slddb-package

Conversation

@bmaranville
Copy link
Copy Markdown
Contributor

This PR

  • creates minimal package infrastructure:
    • pyproject.toml
    • MANIFEST.in
  • adds two installed packages:
    • slddb, which has the api for looking up all items from the database
    • slddb_server a web-based application that uses that api (and others)
  • moves the slddb core code from the orsopy package to this one.
  • unifies the local query function with the webquery function (all api calls, local or remote, use same arguments now)
    • this allows using the local slddb the same exact way as the centralized server, once local db is updated

A symmetric PR will be created in the orsopy repository, which removes the slddb code, and adds this slddb package as a requirement.

@coveralls
Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 23208476266

Details

  • 2030 of 2403 (84.48%) changed or added relevant lines in 27 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+84.5%) to 84.478%

Changes Missing Coverage Covered Lines Changed/Added Lines %
slddb/chemical_formula.py 129 130 99.23%
slddb/tests/test_converters.py 188 189 99.47%
slddb/init.py 5 7 71.43%
slddb/dbconfig.py 32 34 94.12%
slddb/converters.py 226 229 98.69%
slddb/material.py 308 316 97.47%
slddb/database.py 210 221 95.02%
slddb/importers.py 18 74 24.32%
slddb/blender.py 18 77 23.38%
slddb/api.py 9 74 12.16%
Totals Coverage Status
Change from base Build 23196505268: 84.5%
Covered Lines: 2030
Relevant Lines: 2403

💛 - Coveralls

@aglavic
Copy link
Copy Markdown
Collaborator

aglavic commented Mar 18, 2026

Hi @bmaranville,
can you explain the reason for this change?

It basically reverts orsopy/#65 (discussed in orsopy/#62) where slddb was put into orsopy for joint distribution.

I don't see the need to make slddb server "distributable" as it is probably only installed once on our website. The slddb package in orsopy has for me two main advantages:

  1. All features of the api are distributed in the ORSO main package when it is installed.
  2. Many features used in the website are also useful off-line and/or reused in the api, so it makes sense to keep it in just one package to avoide code duplication/introduce new bugs.

@bmaranville
Copy link
Copy Markdown
Contributor Author

I agree with you about point 2. - this PR does not duplicate any code (and in fact it reduces duplication a little bit). The entire slddb code is moved from orsopy to this repo, almost completely unchanged.

I also agree that we don't really need the server to be distributable - if you want to have a separate repo just for the server, that's fine with me too. I just thought the slddb repo was a natural place to put the slddb code. We can move the server code to something like a reflectivity/slddb-server repo if you want.

The motivations for this move:

  1. In the previous architecture, some of the logic for the calculation API was implemented only in the server, while most of it lived in orsopy.slddb. It makes sense to me to have all the logic for slddb in one place.
  2. The slddb package would have use outside the orsopy user community, and others may want to install it and use it independently of orsopy
  3. In the new architecture, orsopy would have a dependency on slddb, so it would automatically get installed by everyone installing orsopy, but slddb can be installed independently of orsopy
  4. If we want to distribute a snapshot version of the binary database file, the natural place to do that would be in an versioned package for the slddb (added during the build step).

I disagree that it's useful to put multiple semi-independent libraries into the orsopy package, as this does not significantly make it easier for users to install (pip install is very easy), but it makes it harder to develop those libraries on their own timescales. As you can see in the discussion of reflectivity/orsopy#62, I was opposed to moving slddb into the orsopy package from the beginning, and I think it's worth re-opening that discussion in light of the motivations I have listed above.

All that being said, I started down this path as I was trying to fix an issue where the local query function is currently unable to handle e.g. DNA and RNA lookups and those are sent to the webquery instead. I have learned enough about where everything is (in the process of making this PR) that I think I could fix that issue in-place without moving the slddb code to a new repo.

@aglavic
Copy link
Copy Markdown
Collaborator

aglavic commented Mar 19, 2026

I'm fine with either solution (orsopy.slddb combined vs slddb as separate orso package). I don't think installing orsopy for slddb is an issue, as we have basically no extra requirements (orsopy requires pyyaml, nothing else), but I see you point about usage outside of orso.

In case we do this separation, I'd prefer two separate repositories for the server and slddb as there are often purely web-related fixes I need to do on the server that don't change any business-logic. Refactoring all actual logic outside the server is also a good idea. (Now thinking about it a bit I'm no longer sure about separation, I'll look through the code a bit more to make up my mind. Maybe having a coupled server and slddb library version makes some sense.)

Can you check with the ORSO crowd (e.g. by re-opening orsopy#62) to get consensus on this? I think it should also include some kind of backward compatible package resolution code to not break dependable programs. (E.g. GenX is using orsopy.slddb for material resolution.)

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.

3 participants