You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "vchiapaikeo (via GitHub)" <gi...@apache.org> on 2023/10/11 09:25:20 UTC
[PR] Add description to pools view [airflow]
vchiapaikeo opened a new pull request, #34862:
URL: https://github.com/apache/airflow/pull/34862
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: [#ISSUE](https://github.com/apache/airflow/issues/34751)
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Expose description column in the pools view
Before:
<img width="1440" alt="image" src="https://github.com/apache/airflow/assets/9200263/d40e2c9a-551a-4385-8e1c-7a8efd0056f4">
After:
<img width="1440" alt="image" src="https://github.com/apache/airflow/assets/9200263/79c69315-049a-4b94-85b6-257b68141342">
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add description to pools view [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #34862:
URL: https://github.com/apache/airflow/pull/34862#issuecomment-1764143121
Yeah I was wondering if Flask Appbuilder has a solution to this without needing to add something to the model. In Django admin you can add a method on the _view_ class and point a column to it, for example. But I wouldn’t be surprised if there’s nothing similar; F.A.B. is not particularly known for being flexible… If you don’t see anything straightforward I can live with just adding custom styling like Variable for now and wait for people to complain (if they ever do).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add description to pools view [airflow]
Posted by "vchiapaikeo (via GitHub)" <gi...@apache.org>.
vchiapaikeo commented on PR #34862:
URL: https://github.com/apache/airflow/pull/34862#issuecomment-1766614054
Did a bit of research on FAB and am not seeing anything obvious that would allow us to modify the underlying query being executed against the model. Rebasing and I think this should be good to go provided tests continue to pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add description to pools view [airflow]
Posted by "vchiapaikeo (via GitHub)" <gi...@apache.org>.
vchiapaikeo commented on PR #34862:
URL: https://github.com/apache/airflow/pull/34862#issuecomment-1763562897
@uranusjr , what are your thoughts on the previous comment? Sorry - forgot to tag ya.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add description to pools view [airflow]
Posted by "vchiapaikeo (via GitHub)" <gi...@apache.org>.
vchiapaikeo commented on PR #34862:
URL: https://github.com/apache/airflow/pull/34862#issuecomment-1761575624
Ah I see. Wasn't sure if you were referring to browser memory or server memory. I [profiled the render template code](https://gist.github.com/vchiapaikeo/b3b86f0566e8a52c8587a4ca7f34e054#file-pool_list_v2-txt-L234-L251), and I'm not seeing an obvious place to add this. Are you thinking of something more custom on the sqlalchemy model? [column_property](https://docs.sqlalchemy.org/en/14/orm/mapping_columns.html#using-column-property-for-column-level-options) or [@hybrid_property](https://docs.sqlalchemy.org/en/14/orm/extensions/hybrid.html#defining-expression-behavior-distinct-from-attribute-behavior) come to mind but may be a bit overkill.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add description to pools view [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal merged PR #34862:
URL: https://github.com/apache/airflow/pull/34862
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add description to pools view [airflow]
Posted by "vchiapaikeo (via GitHub)" <gi...@apache.org>.
vchiapaikeo commented on code in PR #34862:
URL: https://github.com/apache/airflow/pull/34862#discussion_r1357641912
##########
airflow/www/static/css/bootstrap-theme.css:
##########
@@ -5945,6 +5945,12 @@ button.close {
.affix {
position: fixed;
}
+.truncate-text {
Review Comment:
Ooo right, good call! I want to say that I tried this but this stylesheet wasn't being used in this view. In any case, let me get rid of this altogether because Elad's suggestion to check out `VariableModelView` that leverages `list_template` looks a lot cleaner!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add description to pools view [airflow]
Posted by "vchiapaikeo (via GitHub)" <gi...@apache.org>.
vchiapaikeo commented on PR #34862:
URL: https://github.com/apache/airflow/pull/34862#issuecomment-1760648504
Ah yes, `VariableModelView` uses a custom FAB Model [list_template](https://flask-appbuilder.readthedocs.io/en/latest/templates.html#widgets) and injects additional styling there. We can do something similar to get column resizing in `PoolModelView`:
<img width="1440" alt="image" src="https://github.com/apache/airflow/assets/9200263/fb21c729-b7fe-4a06-b6a8-c8d55246a22c">
However, to @uranusjr's point, we are still loading a potentially long value into memory. @uranusjr , when you say `do truncation when the rows are selected`, what do you think about adding a custom formatter and doing the slicing there?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add description to pools view [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #34862:
URL: https://github.com/apache/airflow/pull/34862#issuecomment-1759294203
> I feel we should trim the string a bit, the description can be extremely long.
What do we do in the Variable page? We have description column there as well.
@vchiapaikeo maybe we already have it handled there and we can just copy it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add description to pools view [airflow]
Posted by "vchiapaikeo (via GitHub)" <gi...@apache.org>.
vchiapaikeo commented on PR #34862:
URL: https://github.com/apache/airflow/pull/34862#issuecomment-1758778346
Good call. What about some additional styling?
<img width="1438" alt="image" src="https://github.com/apache/airflow/assets/9200263/0c9bcb63-342d-4e70-b726-a52de7388283">
<img width="1440" alt="image" src="https://github.com/apache/airflow/assets/9200263/e757d8c1-3007-4c52-b750-015f2b7a9659">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add description to pools view [airflow]
Posted by "utkarsharma2 (via GitHub)" <gi...@apache.org>.
utkarsharma2 commented on code in PR #34862:
URL: https://github.com/apache/airflow/pull/34862#discussion_r1356638328
##########
airflow/www/static/css/bootstrap-theme.css:
##########
@@ -5945,6 +5945,12 @@ button.close {
.affix {
position: fixed;
}
+.truncate-text {
Review Comment:
@vchiapaikeo This doesn't look like the right file to host this CSS since it's Bootstrap's CSS if we upgrade we are likely to change this file.
Can we please move it to `www/static/css/main.css` instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add description to pools view [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #34862:
URL: https://github.com/apache/airflow/pull/34862#issuecomment-1757314755
I feel we should trim the string a bit, the description can be _extremely_ long.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add description to pools view [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #34862:
URL: https://github.com/apache/airflow/pull/34862#issuecomment-1759182676
Is it possible to also do truncation when the rows are selected? Loading a long value into memory can be slow itself and bring down the entire view. I know it can be done with `substr` but the FAB model adds a layer on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add description to pools view [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #34862:
URL: https://github.com/apache/airflow/pull/34862#issuecomment-1760710502
No, I’m thinking if it’d be posible to apply `substr` on the column when the rows are selected from the database. That’s where the long strings get loaded. When you reach the renderer, the strings are already in memory and would already cause load on the server; a custom renderer is still better than only CSS (some bytes saved transfering HTML) but not too much different at that point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org