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

Revert inline threshold option #610

Merged
merged 6 commits into from
Nov 16, 2018

Conversation

drdavella
Copy link
Contributor

@drdavella drdavella commented Nov 16, 2018

This feature has proved to be too problematic to be supported in an official release. The semantics are unfortunately rather complicated due to the fact that the same AsdfFile object can be used both for reading and for writing. It also turns out that there already exists an option to write_to called auto_inline that serves a similar purpose, although I believe it may not be working properly in all cases (see #599).

Also, to complicate matters further, neither inline_threshold nor auto_inline are idempotent, meaning that both modify the underlying AsdfFile object in undesirable ways. This has exposed some significant design warts in the asdf API and implementation that need to be addressed, although that will be difficult and will probably lead to significant user-facing changes.

I hope to revisit this and address the issues in an upcoming PR, but I have begun to consider whether we need to make some significant design changes for 3.0.

cc @jdavies-st

I don't want to eliminate the tests entirely since they will be useful
for when we make the updates to auto_inline.
This reverts commit deac0cd.
@drdavella drdavella added this to the v2.3.0 milestone Nov 16, 2018
@drdavella drdavella self-assigned this Nov 16, 2018
@stsci-bot
Copy link

stsci-bot bot commented Nov 16, 2018

Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog. I help make sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

1 similar comment
@stsci-bot
Copy link

stsci-bot bot commented Nov 16, 2018

Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog. I help make sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 94.764% when pulling 8b4be8c on drdavella:revert-inline-threshold into 34030bb on spacetelescope:master.

@drdavella
Copy link
Contributor Author

drdavella commented Nov 16, 2018

This PR retains the tests that were added and also retains a helper function for deciding when to inline since those will be useful for any future work and I don't want to lose them.

@drdavella drdavella merged commit 96058b8 into asdf-format:master Nov 16, 2018
@drdavella drdavella deleted the revert-inline-threshold branch November 16, 2018 21:05
drdavella added a commit to drdavella/asdf that referenced this pull request Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants