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

Move code from boost.* to boost_adaptbx.*, #458 #470

Closed
wants to merge 3 commits into from

Conversation

Anthchirp
Copy link
Member

provide redirects that throw a FutureWarning on import to aid in
migration.

boost.python.injector was not migrated as it is deprecated anyway
(#386). The DeprecationWarning was upgraded to a FutureWarning instead.

This is step 1 towards resolving the boost python namespace conflict (#458).

I will prepare a changeset for existing code in cctbx_project as well.

provide redirects that throw a FutureWarning on import to aid in
migration.

boost.python.injector was not migrated as it is deprecated anyway
(#386). The DeprecationWarning was upgraded to a FutureWarning instead.

This is step 1 towards resolving the boost python namespace conflict (#458)
obtained via

find . -type f -name "*.py" -print0 | xargs -0 sed -i 's/boost\.python/boost_adaptbx.python/g'
find . -type f -name "*.py" -print0 | xargs -0 sed -i 's/boost\.optional/boost_adaptbx.optional/g'
find . -type f -name "*.py" -print0 | xargs -0 sed -i 's/boost\.std_pair/boost_adaptbx.std_pair/g'
find . -type f -name "*.py" -print0 | xargs -0 sed -i 's/boost\.rational/boost_adaptbx.rational/g'
find . -type f -name "*.py" -print0 | xargs -0 sed -i 's/boost\.tuple/boost_adaptbx.tuple/g'
find . -type f -name "*.py" -print0 | xargs -0 sed -i 's/from boost import/from boost_adaptbx import/g'
git checkout boost

and fixing one test in boost_adaptbx/tests/tst_rational.py
@Anthchirp
Copy link
Member Author

Anthchirp commented Apr 26, 2020

Ran tests for scitbx, libtbx, iotbx, cctbx, rstbx before and after changeset.
FutureWarnings were displayed as expected and tests passed quietly after applying the changeset.

@Anthchirp
Copy link
Member Author

will merge this next week if no objections

Copy link
Member

@bkpoon bkpoon left a comment

Choose a reason for hiding this comment

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

So I think dumping everything into the main boost_adaptbx directory will make it messy in the future to add additional Boost components. And any file reorganization will require another round of API changes which is not helpful. Please retain the old boost_adaptbx/boost file structure that was discussed in #458 so that the mapping of Boost components matches boost_adaptbx.boost.<component>. And once this is agreed upon, we should have an additional week before this is merged so that all developers can make the relevant changes to their repositories without having DeprecationWarnings show up everywhere.

@Anthchirp
Copy link
Member Author

Hmm. Are you really, really sure about this?

I would just like to point out that what you call "messiness" in the boost_adaptbx directory means with this patch it will gain 5 .py files for a grand total of 9 .py files (for comparison: libtbx contains 81).

This "cleanliness" will come at a high price though; Instead of

import boost_adaptbx.python
boost_adaptbx.python.something

we have to write

import boost_adaptbx.boost.python
boost_adaptbx.boost.python.something

everywhere.
That translates into a lot of boosting: 475 * 2 = 950 extra boost.s in the cctbx_project repository alone. That's 6 kB worth of boost. and that is just for all instances of boost.python. I didn't count the others and didn't look in any other repository. It also translates into extra effort when reading or writing any of the 233 affected files.

In contrast, the boost_adaptbx and boost directories have only ever seen 415 commits (43 in the last year, a lot of then Python 3 related), so it's not like this code sees a lot of activity, so I assert that the mental load saved by a cleaner boost_adaptbx directory is negligible.

Personally, I find redundancy in the import namespace name quite annoying and redundant and I'm very much onboard with PEP 20 on this:

Flat is better than nested.

As for the DeprecationWarnings - well, it's the point of DeprecationWarnings to flag the use of deprecated APIs 😄, plus they only show up in tests (does libtbx testing even show them? I honestly don't know). I'm happy to disable them in the PR though and enable them a week later. Announcing the change before the PR is merged makes no sense because people won't be able to make the change yet.

@bkpoon bkpoon mentioned this pull request Aug 14, 2020
@bkpoon
Copy link
Member

bkpoon commented Aug 14, 2020

This "cleanliness" will come at a high price though; Instead of

import boost_adaptbx.python
boost_adaptbx.python.something

we have to write

import boost_adaptbx.boost.python
boost_adaptbx.boost.python.something

everywhere.

That is not quite true. If people want to shorten it, they can do what numpy and matplotlib do with import something as something_else. For example,

import numpy as np
import matplotlib.pyplot as plt
import boost_adaptbx.boost.python as bp

bp.something

I made updates to your branch in fix-boost and created a new pull request (#516) that moves the boost directory back to boost_adaptbx.

@bkpoon bkpoon closed this Aug 14, 2020
@Anthchirp
Copy link
Member Author

I offered to address this in this PR and was waiting for an answer to my question for the past three months. I don't see why you needed to open a new PR.

@bkpoon
Copy link
Member

bkpoon commented Aug 14, 2020

Sorry, I did not think you wanted to make the changes since it sounded like you were arguing that adding 6 kB of "boost" to a 214 MB repository (110 MB without the .git directory) was too much. It was also not clear how it translates to extra effort since the changes are done programmatically as a search and replace.

I made a new branch since I wanted to keep your contributions but not modify your existing branch, so I made a new pull request.

@Anthchirp Anthchirp deleted the boost-to-boost_adaptbx branch September 18, 2020 22:09
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

2 participants