damccorm commented on code in PR #35215:
URL: https://github.com/apache/beam/pull/35215#discussion_r2138102781
##########
sdks/python/apache_beam/ml/rag/ingestion/alloydb.py:
##########
@@ -14,39 +14,31 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-import json
-import logging
from dataclasses import dataclass
-from dataclasses import field
from typing import Any
-from typing import Callable
from typing import Dict
from typing import List
-from typing import Literal
-from typing import NamedTuple
from typing import Optional
-from typing import Type
-from typing import Union
-import apache_beam as beam
-from apache_beam.coders import registry
-from apache_beam.coders.row_coder import RowCoder
-from apache_beam.io.jdbc import WriteToJdbc
-from apache_beam.ml.rag.ingestion.base import VectorDatabaseWriteConfig
-from apache_beam.ml.rag.types import Chunk
-
-_LOGGER = logging.getLogger(__name__)
+from apache_beam.ml.rag.ingestion.jdbc_common import ConnectionConfig
+from apache_beam.ml.rag.ingestion.jdbc_common import WriteConfig
+from apache_beam.ml.rag.ingestion.postgres import ColumnSpecsBuilder
+from apache_beam.ml.rag.ingestion.postgres import PostgresVectorWriterConfig
+from apache_beam.ml.rag.ingestion.postgres_common import ColumnSpec
+from apache_beam.ml.rag.ingestion.postgres_common import ConflictResolution
@dataclass
class AlloyDBLanguageConnectorConfig:
Review Comment:
Since there are API changes, lets:
1) Create a GitHub issue with an explanation of the changes and exactly what
a user needs to do to update their config
2) Add an entry to CHANGES.md as a breaking change which links to the issue
for full upgrade details.
We will also need a notebook update, but that can wait until after this
PR/the release.
##########
sdks/python/apache_beam/ml/rag/ingestion/alloydb_it_test.py:
##########
@@ -253,111 +111,32 @@ def tearDownClass(cls):
if hasattr(cls, 'conn'):
cls.conn.close()
- def test_default_schema(self):
Review Comment:
LMK if you think that will cause us to miss the release cut though. One
option would be to cut them for now and add them back once the PR is in.
##########
sdks/python/apache_beam/internal/gcp/auth_test.py:
##########
@@ -25,6 +25,7 @@
try:
import google.auth as gauth
+ import google_auth_httplib2 # pylint: disable=unused-import
Review Comment:
Any reason we need this?
##########
sdks/python/apache_beam/ml/rag/ingestion/alloydb_it_test.py:
##########
@@ -253,111 +111,32 @@ def tearDownClass(cls):
if hasattr(cls, 'conn'):
cls.conn.close()
- def test_default_schema(self):
Review Comment:
Any reason to remove these tests? Even if they're now redundant, I'd vote we
leave them/modify them to work for the scope of this PR as a sanity check that
this refactor works well. It will also give a nice diff on the UX changes
##########
sdks/python/apache_beam/ml/rag/ingestion/alloydb.py:
##########
@@ -14,39 +14,31 @@
# See the License for the specific language governing permissions and
# limitations under the License.
-import json
-import logging
from dataclasses import dataclass
-from dataclasses import field
from typing import Any
-from typing import Callable
from typing import Dict
from typing import List
-from typing import Literal
-from typing import NamedTuple
from typing import Optional
-from typing import Type
-from typing import Union
-import apache_beam as beam
-from apache_beam.coders import registry
-from apache_beam.coders.row_coder import RowCoder
-from apache_beam.io.jdbc import WriteToJdbc
-from apache_beam.ml.rag.ingestion.base import VectorDatabaseWriteConfig
-from apache_beam.ml.rag.types import Chunk
-
-_LOGGER = logging.getLogger(__name__)
+from apache_beam.ml.rag.ingestion.jdbc_common import ConnectionConfig
+from apache_beam.ml.rag.ingestion.jdbc_common import WriteConfig
+from apache_beam.ml.rag.ingestion.postgres import ColumnSpecsBuilder
+from apache_beam.ml.rag.ingestion.postgres import PostgresVectorWriterConfig
+from apache_beam.ml.rag.ingestion.postgres_common import ColumnSpec
+from apache_beam.ml.rag.ingestion.postgres_common import ConflictResolution
@dataclass
class AlloyDBLanguageConnectorConfig:
Review Comment:
> We will also need a notebook update, but that can wait until after this
PR/the release.
Ideally this would be done during release validation and can double as a
validation step for the dev list.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
