Combined pre-AGU GUI and Calibration Message Work #240
Conversation
robertbartel
force-pushed
the
i/post_agu22/1/agu_gui_calreq
branch
from
9f4757c to
bf9c824
Compare
Fixing logic where pip installs the updated packages (to avoid the entire image rebuild), so make sure deps are ignored (as this was the slow part).
Adding type GET_SERIALIZED_FORM to get the entire serialized state of a dataset.
Refactor to ensure it takes advantage of connection handling via its async context manager logic.
Add new function to get serialized dataset details to DatasetExternalClient.
Fix issue with not acquiring a session, and wrapping some things in try block to catch and log exceptions.
Fixing bug in response to LIST_FILES query, where reason text was not being assembled entirely correctly.
Adding optional offset and length params to abstract interface definition of get_data, and adding get_file_stat abstract method.
Adding support for querying about dataset items.
Updating to support indicating start and size of data for partial transfers.
Optimizing the reloading of datasets on object store manager startup.
Accounting for changes to get_data parameters and implementing get_file_stat.
Updating service to respond to GET_DATASET_ITEMS queries and requests for data with an offset start (i.e. partials).
this also includes some commented out code that *might* need to be added in the future to support data_requirement dissemination
…bjectLockConfiguration error
…en calibration jobs
Fixing bug in deserialization function where processed (i.e., by this type) optional kwargs were being combined into list of additional (i.e., potentially by subtypes) additional kwargs to be passed to __init__, with both lists being passed instead of just the combined.
Fixing setup for TestSimpleHydrofabricSubset that used the MappedGraphHydrofabric class after it was made abstract, replacing its use with its GeoJsonHydrofabric subtype.
robertbartel
force-pushed
the
i/post_agu22/1/agu_gui_calreq
branch
from
bf9c824 to
300c1bf
Compare
Fixing argument handling for those args related to communicating with evaluation service, including the dest name for args, defaults, and use when creating main service handler class.
Fixing imports, class usage, and code style spacing for several things related to evaluation requests within main request-service handler class.
|
@robertbartel, looking at the files the PR affects, I am a little concerned how merging this will affect merging the pydantic refactor work. It might be painful, but we might try doing a dry run of that merge process locally before we do it on github just to see what the consequneces are. I guess a precursor that that thought is, how "complete" does the work this PR introduces need to be? What im really trying to get at is, what does the review process for the PR look like? Speaking for some of my commits in this PR, there are certainly some loose ends that should be tied up or just dropped and have new issues opened to address that work in the future. |
|
@aaraney, I performed a local rebase of the Pydantic changes in the combined branch you'd sent me, In terms of completeness, we expect things to be incomplete at this point. We need to keep track of loose ends and broken things but not make this PR perfect, even for the things within its scope. The reason for the associated PR sequence is that other work builds off this further to continue to iteratively address things. |
@robertbartel, right I am forgetting that you did that. I think the
Thanks for clarifying that. To push this PR forward, ill go ahead and open up TODO issues so we can circle back to incomplete code introduced by this PR. |
|
The more I look at this, the more I am concerned about the amount of dead code and technical debt this PR will introduce. I'm not sure this is avoidable, but I thought I would voice my thoughts either way. |
|
Before we move forward any further with this (including fixing the current conflicts), I am going to spend some time looking at what it will take to untether and re-separate the distinct aspects, as well as the subsequent changes in the related series of PRs (#276, #277, #278, and #279). In most circumstances, I feel the technical debt and dead code @aaraney alluded to earlier would be essentially unavoidable: a side effect of the development process for something this big when it is done in chunks. That's even with this set of changes being bigger and including more than we normally want to see. Here, though, with a fairly big shift in GUI code being planned in #286, the direct need for large parts of this (long-term at least) is now questionable. It may be better at this point to cherry-pick certain things out, even if that comes with a bit of work to resolve resulting conflicts. |
|
@robertbartel, what is the state of this? I've not done much digging, but it seems that the bulk of this work has merged through other PRs. Can we close this? |
|
@robertbartel, is it fair to close this? |
|
@aaraney, I'm not quite ready to yet. Given recent decisions to move away from a React-based GUI, it's not impossible we'd want to come back to some of this, and I'm not sure it's all been merged. Let's wait for now. |
This PR is a combination of the map view GUI work, the dataset management GUI work, and the calibration request message work done prior to AGU Fall Meeting 2022. The original branches for that work have been merged so we don't have lingering, non-aligning, outstanding branches. Also, other subsequent work has been rebased in.
We may still end up with a queue of branches, but if we do, this should at least help us ensure things happen in a series and are thus easier to maintain.
This PR effectively replaces #197, #210, and #217.